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

Major API cleanup #1968

Merged
merged 16 commits into from
Apr 12, 2024
Merged

Major API cleanup #1968

merged 16 commits into from
Apr 12, 2024

Conversation

worksofliam
Copy link
Contributor

@worksofliam worksofliam commented Apr 2, 2024

Changes

This is a fairly large API change, but I think a good one. At a high level, here are the biggest changes:

CCSID and SQL logic

  • runSQL was moved from IBMiContent and into IBMi. It made more sense here as we need to run SQL statements during the connection.
    • The runSQL in IBMiContent was marked as deprecated, but maps to the new implementation in IBMi.
  • qccsid in IBMi was renamed to runtimeCcsid. I thought the previous name was wrong as it could hold both the user CCSID or the QCCSID.
    • Added runtimeCcsidOrigin to determine if the runtimeCcsid was from the user profile or from the system.
  • runtimeCcsid and userDefaultCCSID were made private so they could not be changed in code and only altered during connection.
  • New method getEncoding returns whichever CCSID should be used. This takes both the QCCSID and user default CCSID. So instead of each API determining if our subsequent calls should use QCCSID or default CCSID, this API does it for you. You will see this is used in runSQL.

enableSQL config

  • The user config for enableSQL has been removed.
  • It was replaced with a getter with the same name, enableSQL, in the IBMi class.
  • enableSQL will return true when these two conditions are met: the SQL runner is available and the user has a valid CCSID (which we determine thanks to getEncoding)
  • All references to config.enableSQL have been replaced with connection.enableSQL
  • Notice that there is also a setter for enableSQL. This is used for two reasons:
    • Seems like we actually toggle enableSQL in some places (namely the QSYS FS provider)
    • We need a way to toggle it for our test cases to ensure some components can work without SQL. Though this did expose to me that getMemberList used SQL even when it was disabled. I had to update a test case for that instance. We now expect it to crash when SQL is disabled. Is this a problem?

Todo

  • We need to rename the qccsid prop on IBMi for runtimeCcsid, since we're actually grabbing both the user CCSID first and then the QCCSID if need be. IBMi.ts:622-632
  • Update CCSID warnings to give improved messages.

How to test this PR

  1. Run the test cases.
  2. Try using the extension as normal.
  3. Test our extension API. Seems like the Db2 for IBM i extension continued to work as normal with these API changes.

The failing test case here ('Write tab to member using SQL') is a known issue and is not affected by this PR.

image

Checklist

  • have tested my change
  • have created one or more test cases
  • updated relevant documentation
  • Remove any/all console.logs I added
  • have added myself to the contributors' list in CONTRIBUTING.md

Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@worksofliam worksofliam marked this pull request as ready for review April 3, 2024 12:30
@worksofliam worksofliam requested a review from a team April 3, 2024 12:30
@worksofliam worksofliam added the ready Ready for review label Apr 3, 2024
@worksofliam worksofliam linked an issue Apr 3, 2024 that may be closed by this pull request
4 tasks
@worksofliam worksofliam added this to the 2.10.0 milestone Apr 3, 2024
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@worksofliam worksofliam mentioned this pull request Apr 11, 2024
19 tasks
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
src/api/IBMi.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Looks good to me @worksofliam !
The code is OK (save for a very small suggested change) and it all runs fine.
I guess it's unrelated, but there is the Test getTable (SQL compared to nosql) test that fails because all the numeric fields are strings in tableB, making deepEqual to fail.

// added by liam
Object.keys(tableA[0]).forEach(k => {console.log({k, a: tableA[0][k], b: tableB[0][k], at: typeof tableA[0][k], bt: typeof tableB[0][k]})});

Object.keys(tableA[0]).map(k => ({k, a: tableA[0][k], b: tableB[0][k], at: typeof tableA[0][k], bt: typeof tableB[0][k], mismatch: (typeof tableA[0][k]) !== (typeof tableB[0][k]) }));

Object.keys(tableA[0])
  .map(k => ({k, a: tableA[0][k], b: tableB[0][k], at: typeof tableA[0][k], bt: typeof tableB[0][k], mismatch: (typeof tableA[0][k]) !== (typeof tableB[0][k]) }))
  .filter(r => r.mismatch)

Anyway, just see that small suggestion and we can merge afterwards. Good job, I love a good cleanup!

Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@worksofliam
Copy link
Contributor Author

@sebjulliand Fixed your suggestion - thanks!

I also tried to fix that issue but there is a deeper issue that the CSV parser and our db2Parse handle the results differently 😢 Agreed, another PR.

Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Approved! Merge whenever you want. 😎

@worksofliam worksofliam merged commit 8239670 into master Apr 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API change: removing enableSQL configuration
2 participants