Switch Numbers to Constants

[Nespina24]

Description

Added constant imports throughout 64 src/server files in order to change hard-coded numeric values to fit the listed format in a newly added file src/server/util/httpCodes.js.

Fixes #1573

Type of change

• Note merging this changes the database configuration.
• This change requires a documentation update

Checklist

• I have followed the OED pull request ideas
• I have removed text in ( ) from the issue request
• You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

No limitations.

[huss]

@Nespina24 Thank you for looking into this and the PR. Here are my initial thoughts:

• Just so you know, you can specify a PR as draft when creating or can change it to that on the webpage after it is created. It indicates it is a work in progress but you want feedback or to make it public. You can do that for this PR is you would like and switch it back if/when it is ready for formal review.
• I noticed some files were reformatted a little. The one I saw was the try/catch lines now align differently. I think I prefer the original style that follows the default VSC formatting and it also would not show the lines as having been changed in the PR.
• As for your question about the tests failing, I tried to look into that. When I started OED I see this error in the terminal window:

web-1       | /usr/src/app/node_modules/mocha/lib/mocha.js:108
web-1       |   return (currentContext.before || currentContext.suiteSetup).apply(this, args);
web-1       |                          ^
web-1       | 
web-1       | TypeError: Cannot read properties of undefined (reading 'before')
web-1       |     at exports.before (/usr/src/app/node_modules/mocha/lib/mocha.js:108:26)
web-1       |     at Object.<anonymous> (/usr/src/app/src/server/test/common.js:82:7)
web-1       |     at Module._compile (node:internal/modules/cjs/loader:1256:14)
web-1       |     at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
web-1       |     at Module.load (node:internal/modules/cjs/loader:1119:32)
web-1       |     at Module._load (node:internal/modules/cjs/loader:960:12)
web-1       |     at Module.require (node:internal/modules/cjs/loader:1143:19)
web-1       |     at require (node:internal/modules/cjs/helpers:121:18)
web-1       |     at Object.<anonymous> (/usr/src/app/src/server/util/readingsUtils.js:5:28)
web-1       |     at Module._compile (node:internal/modules/cjs/loader:1256:14)
web-1       | 
web-1       | Node.js v18.17.1
web-1       | [nodemon] app crashed - waiting for file changes before starting...

The Docker web container also indicates an issue and you cannot access OED via localhost:3000 indicating it is not running. The issue is in a node module but the PR shows no changes to that (it shouldn't since we don't include the node_modules directory as part of our GitHub repo) nor the package files that specify versions. I looked and it is running 10.4.0 of mocha and that seemed okay. I then switched to the development branch and retried but had no issues/errors and OED ran fine. The package-lock.json and the package.json in node_modules/ for mocha both list version 10.4.0 as in your PR so I'm unclear on why they are running differently. Given this, I wanted to ask if you see the same issue or not. If not, I want to figure out why and know how you ran the tests. If yes, this would likely explain the failing tests but I'm unsure why it is happening at this time.

I'll work with you to figure out what is going on. A quick glance indicated the changes looked okay so that is good.

[Nespina24]

@huss Thanks for taking the time to review my issue.

1. Yes, I get that same error message as the one you listed.

2. I narrowed down the errors of the test to those that were listed under "readings API". For instance, when I ran "npx mocha src/server/test/web/readingsBarGroupBasic.js", I got an error that reads:

1) readings API
readings test, test if data returned by API is as expected
for bar charts
basic for groups
response should have a valid reading, startTimestamp, and endTimestamp:
TypeError: Cannot read properties of undefined (reading 'getConnection')
at prepareTest (src/server/util/readingsUtils.js:46:22)
at Context. (src/server/test/web/readingsBarGroupBasic.js:65:27)
at process.processImmediate (node:internal/timers:476:21)

In all, there are over 100 such testcases that produce this error.

[huss]

@Nespina24

> 1. Yes, I get that same error message as the one you listed.

I think this is a high level problem that needs to be addressed first. It will stop many things from running correctly. It may well be the case that once it is resolved the other issues will go away. Let me know if you want to discuss further or want me to try to help as this is not an issue that I would have expected with this work.

> 2. I narrowed down the errors of the test to those that were listed under "readings API". For instance, when I ran "npx mocha src/server/test/web/readingsBarGroupBasic.js", I got an error that reads:

Normally OED developers use npm not npx but it might be okay. Also, there is an npm script command that OED defines of npm run testSome <<file>> so it would be npm run testSome src/server/test/web/readingsBarGroupBasic.js and that should be used as it does the full test setup specification. Finally, this should be done within the OED Docker web container as specified in developer details web page (you may be doing this already but wanted to let you know).

1. readings API
   readings test, test if data returned by API is as expected
   for bar charts
   basic for groups
   response should have a valid reading, startTimestamp, and endTimestamp:
   TypeError: Cannot read properties of undefined (reading 'getConnection')
   at prepareTest (src/server/util/readingsUtils.js:46:22)
   at Context. (src/server/test/web/readingsBarGroupBasic.js:65:27)
   at process.processImmediate (node:internal/timers:476:21)

> In all, there are over 100 such testcases that produce this error.

I think this error is because the DB API connection setup is receiving an undefined value. My suspicion is that OED is not properly running so there is no DB to talk to. Once the issue above is resolved it probably will go away.

[Nespina24]

First of all, apologies for the late response. My laptop broke which took a while to get everything sorted out.

Next, I have been attempting to get a database connection as you said, but so far whenever I run the command you specified, I get the following error:

npm run testsome src/server/test/web/readingsBarGroupBasic.js

> open-energy-dashboard@1.0.0 testsome
> cross-env NODE_ENV=test mocha --timeout 15000 src/server/test/web/readingsBarGroupBasic.js

(node:36968) Warning: Accessing non-existent property 'expect' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
(node:36968) Warning: Accessing non-existent property 'testDB' of module exports inside circular dependency


  readings API
    readings test, test if data returned by API is as expected
      for bar charts
        basic for groups
          1) "before each" hook for "response should have a valid reading, startTimestamp, and endTimestamp"


  0 passing (24ms)
  1 failing

  1) "before each" hook for "response should have a valid reading, startTimestamp, and endTimestamp":

  AggregateError [ECONNREFUSED]:
      at internalConnectMultiple (node:net:1139:18)
      at afterConnectMultiple (node:net:1712:7)

[omarraf]

Hi! I looked into the test failures and think I found the root cause. When HTTP_CODE was added to readingsUtils.js and the 18 production route files started importing it, a circular dependency was introduced (as mentioned in the error screenshot @Nespina24 shared above):

test/common.js → app.js → routes/*.js → readingsUtils.js → test/common.js

So when a test loads common.js, Node starts evaluating it, hits app.js, which loads all the routes, which load readingsUtils.js, which tries to require('../test/common'), but common.js is still mid-evaluation. Node returns its partially-built exports (an empty object), so testDBcomes back as undefined. So when prepareTest later calls testDB.getConnection(), it crashes.

I think a potential fix would be to move the HTTP codes into their own util file with no dependencies, (i.e src/server/util/httpCodes.js), instead of readingsUtils.js, and the route files would just import from the newly created file. That would break the cycle and let common.js fully evaluate before anything tries to use it.

What do you all think? @Nespina24 @huss

[huss]

@omarraf (& @Nespina24) First, I want to thank you for doing your analysis to trace down the issue. Second, I think your proposed solution is fine.

I was curious about this so I looked into it some. I double checked and this issue was not present in development but it is in your PR. This made me wonder if it was the testing files that were the direct cause. From what I have seen, I think the more precise reason for this issue is that to get the HTTP_CODE that you added to the routes causes the test includes to be loaded. I don't think that should happen. Given it is in the a util/ directory that isn't a test directory, I think your proposal is the right decision. I also noticed that it really is not a single code but a group of them. So, your proposed name of httpCodes.js seems like a good choice. If you are willing, I think changing the name of the const to match (HTTP_CODES) would also make sense. Given you have to modify every require anyway, I'm hoping that is not significant work.

I welcome any thoughts or other ideas. Again, thanks for your analysis that pointed out the source of the issue. Mine are just another spin on it.

[Nespina24]

@omarraf @huss This sounds good to me. Just to clarify, would it be best to update only the imports for the src/server/routes files as the original issue specified or update all imports of the HTTP codes to the new file? I'm ok with either option but I think that updating the other imports could be its own issue if needed.

[huss]

> Just to clarify, would it be best to update only the imports for the src/server/routes files as the original issue specified or update all imports of the HTTP codes to the new file? I'm ok with either option but I think that updating the other imports could be its own issue if needed.

@Nespina24 In general, I don't think it is a good idea to have two versions of the same file. I'm fine if you want to open an issue and link it to this PR if you prefer. If you do want to open a separate PR then I think that one needs to be done first and then use the new include in this PR. I personally would do them both in one PR to keep it simpler but I will take two if you prefer. (Your choice if you would like to open an issue.)

[Nespina24]

> In general, I don't think it is a good idea to have two versions of the same file. I'm fine if you want to open an issue and link it to this PR if you prefer. If you do want to open a separate PR then I think that one needs to be done first and then use the new include in this PR. I personally would do them both in one PR to keep it simpler but I will take two if you prefer. (Your choice if you would like to open an issue.)

I think there's a bit of miscommunication here. I was referencing the fact that multiple files outside of src/server/routes are currently pointing to the HTTP_CODE constants in readingsUtil.js, but I was unsure whether to address them in this pull request or not. I was confused by the "two versions of the same file" you mentioned, as I thought the imports would all point to src/server/util/httpCodes.js eventually.

Of course, I may be mistaken in that assumption and you might want to keep the constants that are currently in readingsUtil.js for the other files. I want to be clear before I begin working on the implementation.

[huss]

I'm sorry for the confusion. Let me try again. I believe you are proposing to create the HTTP codes in httpCodes.js. I was saying that they should not also remain (as a second set) in the current include file (the two versions you asked about). If this is done then I think all the imports need to be updated (hopefully with a plural in the constant name) for this location. So, yes, this means that the new ones in the routes you created and the older ones in the test files all would need updating. I was advocating somewhat to update the current PR to cover that but wanted to say that two PRs are okay as long as the first one changes the location and fixes the existing usages. Then the second would use the new location for the routes you modified already. Does this help and make sense? I'm open to other ideas.

[huss]

> I double checked and this issue was not present in development but it is in your PR.

@Nespina24 & @omarraf I wanted to update this while I am still working on it. I recently posted to the OED developer Discord channel that the recent version of development is causing install failures. As stated, I see it too so I was wrong about thinking there was not an issue with development (it had to do with how I was testing - sorry). Given this, I tried your proposed solution to see if it helped. It does not seem to resolve the underlying issue assuming I did it all right. If I bring up OED without recreating the DB (see Discord msg) then it works and the app is fine. However, every test fails. I'm trying to figure out why but I wanted to post this much while the effort is ongoing.

[Nespina24]

Edit: I just realized after committing, I changed the incorrect routes files and did not change the error. Apologies for the confusion. I will fix the correct files over the following days.

@omarraf @huss I tried adding the new file as specified, but I had a similar issue to @huss where not all, but a majority of the npm tests started failing due to various reasons, such as various TypeError's:

120) Login Parameter Validation
       Type Safety
         should reject non-string passwords:
     TypeError: Cannot read properties of undefined (reading 'BAD_REQUEST')
      at Context.<anonymous> (src\server\test\routes\loginParamsTest.js:322:32)
      at process.processImmediate (node:internal/timers:491:21)

  121) Log Routes
       Basic API validation
         should return 200 for valid info message:
     TypeError [ERR_HTTP_INVALID_HEADER_VALUE]: Invalid value "undefined" for header "token"
      at ClientRequest.setHeader (node:_http_outgoing:702:3)
      at Request.request (node_modules\superagent\lib\node\index.js:801:39)
      at Request.end (node_modules\superagent\lib\node\index.js:919:8)
      at C:\ForNathan\GitHub\OED\node_modules\superagent\lib\request-base.js:266:12
      at new Promise (<anonymous>)
      at RequestBase.then (node_modules\superagent\lib\request-base.js:250:31)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)

Before jumping to conclusions, I wanted to run this code past you guys in case the implementation was missing something, but it's possible that this change doesn't completely resolve the issue at hand.

[omarraf]

@Nespina24 Could you try adding brackets to the export at the end of the httpCodes file? Like module.exports = { HTTP_CODES }; and try testing it again to see if that fixes anything

[huss]

@Nespina24 I looked at the patch I created yesterday to try to fix all the errors and included a fix for this because I was uncertain as to what was going on. I am not seeing this error. Also, I need to look at it more but it seems the issue I found with startup just disappeared - not sure why. Let me know if your issue does not clear so we can figure out why.

[Nespina24]

@huss @omarraf I realized in my previous commit that I changed the incorrect files, as I mistakenly changed the imports in src/server/test/routes instead of src/server/routes. After making the changes to the correct files, the TypeErrors went away and most of the tests passed. I am assuming the standalone errors are due to some separate issue since other test cases in the routes folder went through. The test cases returned:

220 passing (5m)
 4 pending
 13 failing

I haven't run the tests in the default state without the import changes, so there might be some possible issues this change introduced. As always, I am open to any suggestions or next steps forward for the issue. Thanks for your understanding.

[huss]

@Nespina24 & @omarraf I was trying to consider the new failing tests and noticed something but I don't know if it is related or not. src/server/util/readingsUtils.js still seems to have HTTP_CODE. A global search found it used in a number of files such as src/server/test/routes/groupsParamsTest.js. In that one the import seems correct but not the uses. In others it has different combinations of usage. I hope I'm not missing something but it seems something is off. Note when I did this change I carefully used a global replace to do it systematically. Please let me know if I can help.

Also, the merging of development, per a comment on the OED developer Discord channel, breaks my normal startup of OED. I am able to run it another way so I can try tests if that is needed but I wanted to get your take on the thought above.

[omarraf]

Yeah just adding on to what @huss mentioned, one of the issues is in src/server/test/routes/groupsParamsTest.js on line ~24 where it should be 'HTTP_CODES.OK' and 'HTTP_CODES.BAD_REQUEST' instead of 'HTTP_CODE'. This is the only one I saw but there could be more, try running tests after that. @Nespina24

[Nespina24]

@huss @omarraf My last commit should have gotten the remaining files that use the 'HTTP_CODES' import. I still get a few test failures, such as in src\server\test\routes\logsRouteTests.js but I also get the same errors when I run it on the development branch, leading me to believe the imports are working as intended.

I'm not sure what the next steps should be at this point, so any review or feedback is appreciated.

[huss]

I still get a few test failures, such as in src\server\test\routes\logsRouteTests.js but I also get the same errors when I run it on the development branch, leading me to believe the imports are working as intended.

@Nespina24 & @omarraf When I run this locally, I only occasionally see the failures. I am trying to figure it out but I'm assuming this isn't an issue with this PR. Thus, I don't think you need to worry about it for this PR. I think it always passes on GitHub CI so it should not hold this up. (This may indicate it is a timing/speed issue with the code.)

Know that I'm looking at other aspects of the PR to move it forward. To get all the comments straight, I tried to use strike-through on anything that I think is resolved in previous comments.

[huss]

@Nespina24 Thank you for the updated code. First, I think this is close enough that it would be good to have it converted from a draft PR to a regular one. Is that okay with you? If so, will you do it or would you like me to?

I have now gone through every file and change to look at them. I want to thank you for being careful as I found virtually nothing. I have made a few, minor comments to consider.

I also swept the code to try to catch any places where numbers were used instead of the new constants for return code. Files with codes still present:

• src/server/services/csvPipeline/success.js: 200, 400
• src/server/test/web/csvPipelineTest.js:200, 400
• src/server/test/web/groups.js: 200, 203, 400, 401, 403
• src/server/test/web/login.js: 200, 401
• src/server/test/web/maps.js: 200, 403, 500
• src/server/test/web/meters.js: 200, 403, 500
• src/server/test/web/obviusTest.js: 200, 401, 406
• src/server/test/web/preferencesTest.js: 200, 403
• src/server/test/web/usersTest.js: 200, 400, 403
• src/server/test/web/version.js: 200
• src/server/services/csvPipeline/CustomErrors.js: 400
• src/server/app.js: 404
• src/server/services/csvPipeline/failure.js: 500
• src/server/services/csvPipeline/saveCsv.js: 500
• src/server/services/csvPipeline/uploadMeters.js: 500

Would you be interested/willing to fix these up as part of this PR or would you prefer someone else to take that on? Note this does not include any open PRs and that will be done as work outside this effort/PR. I'm asking since it would be nice to get this merged in the near future so other places that are coming can use the new constants in that new code.

Please let me know if anything is not clear, you have thoughts/questions or need any help.

[huss]

When I run this locally, I only occasionally see the failures. I am trying to figure it out but I'm assuming this isn't an issue with this PR.

After adding debug statements, I think I may have an idea of why this is happening. I won't be sure until a fix is figured out and tested. Since this started here, I'm adding some details. It is tricky to get this to fail but one time there was only the first log message and not the later one that the test expects so it does not get any. After thinking about it, I wondered if the code was not waiting on the log msg to be inserted. Sure enough, OED does not hold up the code to log errors so it can continue. Thus, there is currently no guarantee that the log message will be in the DB after the log route returns. I suspect it does not happen very often but sometimes the request to get the filtered logs happens to execute before the new log is entered. Since GitHub is faster than most developer machines it is not seen in that case but sometimes a local machine takes too long. Again, further testing is needs to make sure this is really happening.

Now the fix is uncertain. One bandaid approach (not a good way) is to put in a delay in the test so the second request is delayed and hope/assume it has completed. Another is to retry after a delay when the first attempt does not get the log message. A better fix would be to resolve the issue but OED does not want to wait for all logs to be in the DB (await) before proceeding. Maybe there could be a switch to await during testing but not at other times but this is a non-trivial change. Again, more thought and work in the future is needed.

I welcome any thoughts.

[Nespina24]

@huss & @omarraf It's a relief to hear that you can pass src\server\test\routes\logsRouteTests.js since I always had it fail on my local machine, so it sounds like the import didn't change it after all. I will keep your solutions in mind if I do any related work on OED.

I had one last question about the pull request description: Do I need to tick off any of "Note merging this changes the database configuration" or "This change requires a documentation update" or should I leave them unchecked? I was wondering if the addition of the file src/server/util/httpCodes.js warranted a change to either of them.

[huss]

I had one last question about the pull request description: Do I need to tick off any of "Note merging this changes the database configuration" or "This change requires a documentation update" or should I leave them unchecked? I was wondering if the addition of the file src/server/util/httpCodes.js warranted a change to either of them.

Thank you for asking. The documentation refers to the user help files (most of the time). Since this did not change how the code functions but how it is implemented there is no need to check that box. Once this is merged, all developers will see the change and it should be easy to use this in any new code. As for database configuration, this does not touch the setup of the DB so that does not need to be checked.

I have this on my list to review again. If you want any advise on other OED work you could do then just let me know.

[huss]

For the record, I just pushed a commit for app.js to fix a couple of codes that I missed before so I am fixing. It was much easier to check once most had been updated in this PR.

[huss]

@Nespina24 Thank you for the updated code. Review and testing found this work as desired. I want to thank you for all of your efforts and doing extra work to make this a holistic solution., Congratulations on your first accepted contribution to OED.