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

API change: use default CCSID to run SQL queries when needed #1828

Merged
merged 10 commits into from
Feb 13, 2024

Conversation

sebjulliand
Copy link
Collaborator

Changes

Running SQL queries in a job whose CCSID is 65535 is bad. This PR tries to avoid conversion errors by running a CHGJOB CCSID(xx) command before every SQL statement run through QZDFMDB2.

In above command, xx is the job's default CCSID which is retrieved at connection time (once then cached).

Noticeable changes:

  • Default CCSID is retrieved using the ACTIVE_JOB SQL service. If it fails, we fall back to using DSPJOB OPTION(*DFNA) and parsing the output.
  • the runSQL method has been simplified a bit. The logic to sanitize the statement about to be run has been put in a separate method in Tools and there is a unit test to check it.
  • QZDFMDB2 allows to run CL command simply by prefixing them with @. It's convenient but it turns out such statements are always run in a separate job. So the method that sanitizes QZDFMDB2 statements will also rewrite every @command into Call QSYS2.QCMDEXC('command').
  • A varchar conversion has been added in the getMemberList function to avoid a 1200 -> 65535 conversion issue, apparently unavoidable even with CHGJOB. The member's TEXT field is using CCSID 1200.
  • The Default CCSID has been added to the Report Issue template. It is also reported in the Code for i output if QCCSID is bad.
    image

Checklist

  • have tested my change

sebjulliand and others added 7 commits February 4, 2024 21:36
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Avoid CCSID 1200 to 65535 conversion error

Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
…poses)

Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
@sebjulliand sebjulliand added the enhancement New feature or request label Feb 5, 2024
@sebjulliand sebjulliand requested a review from a team February 5, 2024 12:58
@sebjulliand sebjulliand self-assigned this Feb 5, 2024
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Requested some changes, simple stuff mostly.

I would also appreciate an additional test for runSQL that includes running a CL command and then a statement.

Perhaps something like CHGLIBL and then select * from QSYS2. LIBRARY_LIST_INFO.

src/api/IBMiContent.ts Show resolved Hide resolved
src/api/Tools.ts Outdated Show resolved Hide resolved
Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
@sebjulliand
Copy link
Collaborator Author

Thanks @worksofliam !
I pushed a new test and the name modification. See my comment for your other comment.

Signed-off-by: Seb Julliand <sjulliand@arcadsoftware.com>
@worksofliam worksofliam added this to the 2.7.0 milestone Feb 7, 2024
@worksofliam worksofliam self-requested a review February 9, 2024 23:11
name: `Test @clCommand + select statement`, test: async () => {
const content = instance.getContent()!;

const [result] = await content.runSQL(`@CRTSAVF FILE(QTEMP/UNITTEST) TEXT('Code for i test');\nSelect * From Table(QSYS2.OBJECT_STATISTICS('QTEMP', '*FILE')) Where OBJATTRIBUTE = 'SAVF';`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great test case!

@worksofliam worksofliam self-requested a review February 9, 2024 23:13
@worksofliam
Copy link
Contributor

@sebjulliand Before I leave my final review, I do think as part of this, before we release it especially, is that we should document this at a high level on our CCSID page. Something like... yep, we're setting the CCSID, but you really should set it on your user profile.

@worksofliam worksofliam linked an issue Feb 9, 2024 that may be closed by this pull request
@sebjulliand
Copy link
Collaborator Author

@worksofliam agreed, and maybe add a button to open the URL on the notification that says naught-no-ccsid.

@worksofliam
Copy link
Contributor

@sebjulliand Is that something you want to include in this PR or another time?

@@ -50,7 +50,8 @@ export default class IBMi {
defaultUserLibraries: string[];
outputChannel?: vscode.OutputChannel;
aspInfo: { [id: number]: string };
qccsid: number | null;
qccsid: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am starting to wonder if this qccsid property should be renamed. Years ago we used to only check for QCCSID, but now we also store the job CCSID in this first.

What do you think?

@worksofliam
Copy link
Contributor

Baring my two comments about:

  1. When do we want to update the docs?
  2. qccsid property name

The tests are passing and I am happy with the code.

image

@sebjulliand
Copy link
Collaborator Author

Thanks @worksofliam ! I'll merge now, and open another PR for renaming qccsid to something better and display a better error message when it is 65535.

@sebjulliand sebjulliand merged commit cd8ab5e into master Feb 13, 2024
1 check passed
@sebjulliand sebjulliand deleted the feature/dbCCSID branch February 13, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more notes about locales to CCSID docs
2 participants