Fixed the extractSetOfValueSetsFromLibrary function in CodeService.js…

[sorliog]

CQL JSON-ELM standard defines values sets in Libraries to utilize the valueSets property, as defined in the schema.
The extractSetOfValueSetsFromLibrary in CodeService.js loops through a library formatted in JSON-ELM to find all declared value sets. Previously, it looked the the value sets using library.valuesets. This is not the proper property to lookup value sets, as defined in the afomentioned schema.

In this pull request, I fixed the extractSetOfValueSetsFromLibrary function in CoderService.js to extract value sets contained in the valueSets property (using library.valueSets). Additionally, for backward compatibility, I also use library.valuesets property to extract value sets.

[cmoesel]

Thank you for finding and fixing this. To be honest, I'm not sure how we've come this far without noticing it. I suspect that maybe when this was originally written, the CQL-to-ELM Translator may have exported ELM using valuesets, but I don't think that's been the case for a while...

Our CI found that there is a dependency with a known vulnerability. While it's not directly related to your PR, might you be able to run npm audit fix and update this PR so the CI runs clean? If that's too much trouble, let me know and I will accept as-is and follow up with another PR that fixes the vulnerability.

[sorliog]

@cmoesel Thank you for reviewing my PR. I ran npm audit fix and committed the code in package-lock.json. Please rerun the CI workflow.

[cmoesel]

I apologize. I did not realize before that there is also a prettier code formatting error. To check your code against the lests, linter, and prettier, run:

npm run test:plus

You can automatically fix prettier violations by running:

npm run prettier:fix

This is documented at the bottom of the README.

I apologize for not catching that in my first review.

[sorliog]

@cmoesel The style checks pass on my machine, and the workflow run on Github was not specific as to what caused the failure. I reran the prettier script and committed the code. Hopefully, that solves it.

[cmoesel]

I've tried these changes in some existing projects and I'm sorry to report that they introduce problems in some of those projects.

I've investigated and now I understand why it is the way it is. The library that is passed in is not intended to be the literal JSON ELM library. It is intended to be an instance of the Library class from cql-execution. If you look at the source code, that Library class has a valuesets property (w/ lowercase s). So this code is correct to use valuesets rather than valueSets.

You can construct a Library class by passing in the JSON to the constructor (e.g., new Library(myLibraryElmJson)) -- and then pass that as the library argument in the ensure functions.

BTW -- the errors I ran into when I tried to run it were because library.valuesets is sometimes an empty object ({}), so then Object.values(library.valuesets)[0] on line 244 results in undefined, causing a dereference error on line 248.

[sorliog]

@cmoesel Thank you for the clarification. I assume that if the valuesets property of the Library class was simply renamed to valueSets, it would solve the issue of using both a ELM library or the Library class. But that would require all references in all repos that depend on it to be refactored.
What I don't understand is why reading both library.valuesets and library.valueSets would introduce problems in other projects. After all, if other project use library.valuesets, and my code also reads and processes library.valuesets, it should be compatible?

Regarding the dereference errors, I see what the problem is. I can fix it, if you want to go through with the merge.