Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump codeql/javascript-all and fix breaking changes #170

Merged
merged 15 commits into from
Feb 19, 2025

Conversation

jeongsoolee09
Copy link
Contributor

The old dataflow API was deprecated in 2.3.0. This deprecates the older APIs but immediately makes some APIs such as BarrierGuardNode.blocks invalid as they are renamed without the deprecated qualifier.

Future works

Overhaul the query to use the new APIs before the grace period ends.

The old dataflow API was deprecated in 2.3.0. This
deprecates the older APIs but immediately makes
some APIs such as BarrierGuardNode.blocks invalid
as they are renamed without the `deprecated`
qualifier.
@lcartey
Copy link
Contributor

lcartey commented Feb 12, 2025

@jeongsoolee09 we should also update https://github.com/advanced-security/codeql-sap-js/blob/main/qlt.conf.json.

This one caused a new non-monotonic recursion; it
is not necessary at the moment since it is not
used anywhere apart from a vacuous `import`.
The AMD-style module system is already defined in
UI5.qll as abstract class `UserModule`; so deleting
this won't break the rest of the UI5 queries.
… the

expected results

`SapDefineModule` in its previous form did not extend
`AmdModuleDefinition::Range` which in turn extends standard library's
`Module` that enables its subclasses to be identified with MaD definitions.
The reason the queries have been working was thanks to `UI5AMDModule.qll` that
provided a `SapAmdModule` class extending the `Module` class directly.

The problem with `SapAmdModule` copy-pasted and only slightly modified
the `AMD.qll` in the standard library, so it was out of sync with the
standard library couterpart when the DataFlow library behind it was
overhauled to have a new API. It was causing the majority of the
problems when the `qlpack.yml`s were updated with the latest DataFlow
API and the `SapAmdModule` failed to play nicely with the updated
library modules, emitting new non-monotonic recursion errors.

Therefore, this commit makes the `SapDefineModule` in `UI5.qll` extend
`AmdModuleDefinition::Range` and removes the outdated `SapAmdModule`.
`SapDefineModule`s are AMD-style modules defined with
`sap.ui.define` or `sap.ui.require` function calls that augments the
global AMD `define` functions with the capability of extending another
such module.
These expected files are prepended with a deprecation warning on the old
DataFlow APIs; no changes were occured in the rest of the contents.
This is to publish these QL packs with a new version.
data-douser added a commit that referenced this pull request Feb 18, 2025
Attempt to resolve unit test failures for PR #170 of the
`advanced-security/codeql-sap-js` repo.

Updates the (soon to be replaced) shell-based version of the CDS
extractor as an attempted workaround for a change in cds compiler
behavior when the `-o` or `--dest` options are set for the `cds compile`
CLI command. Forces the cds compmiler to output to the desired .cds.json
file path via stdout.
@data-douser
Copy link
Collaborator

Here is the git diff comparing this PR to #171 :

diff --git a/.github/workflows/run-codeql-unit-tests-javascript.yml b/.github/workflows/run-codeql-unit-tests-javascript.yml
index 8a10d70..3a3ec23 100644
--- a/.github/workflows/run-codeql-unit-tests-javascript.yml
+++ b/.github/workflows/run-codeql-unit-tests-javascript.yml
@@ -81,7 +81,8 @@ jobs:
         run: |
           if ! command -v cds &> /dev/null
           then
-            npm install -g @sap/cds-dk
+            ## Workaround for https://github.tools.sap/cap/issues/issues/17840
+            npm install -g @sap/[email protected]
           fi
 
       # Compile .cds files to .cds.json files.
@@ -97,8 +98,8 @@ jobs:
               echo "I am compiling $cds_file"
               cds compile $cds_file \
                 -2 json \
-                -o "$cds_file.json" \
-                --locations
+                --locations \
+                > "$cds_file.json" 2> "$cds_file.err"
             done
             popd
           done
diff --git a/extractors/cds/tools/index-files.sh b/extractors/cds/tools/index-files.sh
index 092d126..7e8dde8 100755
--- a/extractors/cds/tools/index-files.sh
+++ b/extractors/cds/tools/index-files.sh
@@ -36,12 +36,12 @@ then
     # directory.
     #
     # We also ensure we skip node_modules, as we can end up in a recursive loop
-    find . -type d -name node_modules -prune -false -o -type f \( -iname 'package.json' \) -exec grep -ql '@sap/cds' {} \; -execdir bash -c "grep -q \"^\$(pwd)\(/\|$\)\" \"$response_file\"" \; -execdir bash -c "echo \"Installing @sap/cds-dk into \$(pwd) to enable CDS compilation.\"" \; -execdir npm install --silent @sap/cds-dk \; -execdir npm install --silent \;
+    find . -type d -name node_modules -prune -false -o -type f \( -iname 'package.json' \) -exec grep -ql '@sap/cds' {} \; -execdir bash -c "grep -q \"^\$(pwd)\(/\|$\)\" \"$response_file\"" \; -execdir bash -c "echo \"Installing @sap/cds-dk into \$(pwd) to enable CDS compilation.\"" \; -execdir npm install --silent @sap/[email protected] \; -execdir npm install --silent \;
 
     # Use the npx command to dynamically install the cds development kit (@sap/cds-dk) package if necessary,
     # which then provides the cds command line tool in directories which are not covered by the package.json
     # install command approach above
-    cds_command="npx -y --package @sap/cds-dk cds"
+    cds_command="npx -y --package @sap/[email protected] cds"
 else
     cds_command="cds"
 fi
@@ -51,8 +51,11 @@ echo "Processing CDS files to JSON"
 # Run the cds compile command on each file in the response file, outputting the compiled JSON to a file with
 # the same name
 while IFS= read -r cds_file; do
-    echo "Processing CDS file $cds_file to:"
-    if ! $cds_command compile "$cds_file" -2 json -o "$cds_file.json" --locations 2> "$cds_file.err"; then
+    echo "Processing CDS file $cds_file to: $cds_file.json"
+    # Avoid using the `-o` (or `--dest`) option as it sends output to a new directory, where we want to
+    # output to a file in the same directory as the input file but with a .json extension.
+    if ! $cds_command compile "$cds_file" -2 json --locations > "$cds_file.json" 2> "$cds_file.err"
+    then
         stderr_truncated=`grep "^\[ERROR\]" "$cds_file.err" | tail -n 4`
         error_message=$'Could not compile the file '"$cds_file"$'.\nReported error(s):\n```\n'"$stderr_truncated"$'\n```'
         echo "$error_message"
@@ -101,4 +104,4 @@ unset LGTM_INDEX_INCLUDE
 echo "Extracting the cds.json files"

data-douser and others added 2 commits February 19, 2025 13:19
Updates unit-test and cds-extractor use of the `cds compile` command in
order to avoid the use of the `-o` option, which outputs to a directory
instead of a file, as intended. Replaces use of the `-o` option with a
simple redirect of cds compiler output to stdout, which is redirected to
the indended '.cds.json' file path.

This commit should resolve failures for both code scanning and unit test
workflows for this project/repo.
…cript-dataflow-lib-amend

Run `cds compile` command without `-o` option
@jeongsoolee09
Copy link
Contributor Author

Thanks @data-douser for your contribution!

Copy link
Collaborator

@data-douser data-douser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes LGTM and should unblock a couple of major efforts.
Nice work @jeongsoolee09 !

@jeongsoolee09 jeongsoolee09 merged commit e8d5b09 into main Feb 19, 2025
5 checks passed
@jeongsoolee09 jeongsoolee09 deleted the jeongsoolee09/bump-javascript-dataflow-lib branch February 19, 2025 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants