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

Component API #1985

Merged
merged 48 commits into from
May 14, 2024
Merged

Component API #1985

merged 48 commits into from
May 14, 2024

Conversation

worksofliam
Copy link
Contributor

@worksofliam worksofliam commented Apr 11, 2024

Changes

As we increase our dependence on our own SQL tools, I think it was time we added an API to easily add and maintain different services.

As well as that, this PR does another of other things internally:

  • Renamed runtimeCcsid to jobCcsid and also added qccsid on the IBMi class.
  • Moved IBMiContent out of Instance and into the IBMi class.
  • Checks for the status of the component every time we connect. This data is not cached.
  • the IBMi#runSQL method will, depending on the QCCSID, fetch the data from a CSV using one of two of our new components. This will solve a number of CCSID issues when working with EBCDIC in tables as we read only in UTF8.
    • We fallback to CPYTOIMPF if the statement is 'simple' as it is more performant than SQL_TO_CSV
    • We fallback to CPYTOIMPF if SQL_TO_CSV fails to install
  • IBMiContent#getTable now only uses CPYTOIMPF and does not resolve to SQL when possible. We still use this API to fetch a specific member during the complication process (EVFEVENT file members)
  • Added new tests for multiple encodings. More are welcome.

Nice to haves

Below are some CCSID related issues which would go along well with this PR:

To do

  • Install components if not installed
  • Check the state of a component (installed or not)
  • Improve version control of individual components
  • Added SQL_TO_CSV component, but it does not install
  • Added IFS_WRITE component, but it does not install
  • Inner component dependencies? (SQL_TO_CSV depends on IFS_WRITE)
  • Fix handling of null values in SQL_TO_CSV
  • Write components to a new dedicated library if available
  • Test cases for runSQL API using SQL_TO_CSV
  • Encoding tests (steal from Feature/sql_runners #1983) to verify SQL_TO_CSV
  • Improve type system of ComponentManager
  • Improve performance of getMemberList
  • Convert IFS_WRITE to have a clob parameter over varchar
  • Major API cleanup #1968 merged

How to test this PR

Examples:

  1. Run the test cases

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

worksofliam and others added 5 commits April 11, 2024 13:27
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>
Co-authored-by: Niels Liisberg <nli@system-method.com>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@chrjorgensen
Copy link
Collaborator

@worksofliam @sebjulliand I think it would be great if the extension would

  1. check the component versions installed on the server on first connect after extension update and install any component not there or not up to date
  2. use a versioning method that also covers CL objects - CL programs don't have long comment like SQL objects, and we may introduce non-SQL components later on (we had that for command definitions before it was moved to the CL extension). We could use the object text to have the version information, e.g. 'v1 Code for IBM i: Get something...'.

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 changed the base branch from cleanup/apis to master April 12, 2024 17:37
@worksofliam
Copy link
Contributor Author

@chrjorgensen Per your second point: each component has their own getInstalledVersion method which returns a number. Each component can have their own version checker. So for CL, we might use SQL to get the object text, whereas for SQL objects we might use the comments.

worksofliam and others added 6 commits April 12, 2024 14:16
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Co-authored-by: Niels Liisberg <nli@system-method.com>
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 requested review from sebjulliand and chrjorgensen and removed request for sebjulliand April 15, 2024 17:20
@worksofliam
Copy link
Contributor Author

@codefori/core Any chance I can get a midpoint review on this API changes so far?

@worksofliam
Copy link
Contributor Author

Here are some of the failing tests due to some issues in SQL_TO_CSV that I am going to need some help with:

image image

This one looks like it should be passing?

image

Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@worksofliam worksofliam linked an issue Apr 16, 2024 that may be closed by this pull request
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
@worksofliam
Copy link
Contributor Author

CALL ILEDITOR.SQL_TO_CSV('With MEMBERS As (         SELECT           rtrim(cast(a.system_table_schema as char(10) )) as LIBRARY,           b.avgrowsize as RECORD_LENGTH,           a.iasp_number as ASP,           rtrim(cast(a.system_table_name as char(10) )) AS SOURCE_FILE,           rtrim(cast(b.system_table_member as char(10) )) as NAME,           coalesce(rtrim(cast(b.source_type as varchar(10) )), '''') as TYPE,           coalesce(rtrim(varchar(b.partition_text)), '''') as TEXT,           b.NUMBER_ROWS as LINES,           extract(epoch from (b.CREATE_TIMESTAMP))*1000 as CREATED,           extract(epoch from (b.LAST_SOURCE_UPDATE_TIMESTAMP))*1000 as CHANGED         FROM qsys2.systables AS a           JOIN qsys2.syspartitionstat AS b             ON b.table_schema = a.table_schema AND               b.table_name = a.table_name       )       Select * From MEMBERS       Where LIBRARY = ''QSYSINC''         And SOURCE_FILE = ''QRPGLESRC''                         Order By NAME DESC', '/tmp/vscodetemp-O_jj1aTKjM')

--DB2>
--  ?>
--
-- **** CLI ERROR *****
--         SQLSTATE: 22001
--NATIVE ERROR CODE: -302
--Conversion error on variable or parameter SQLP_L2.FILE_CONTENT.

@worksofliam
Copy link
Contributor Author

The cause of the conversion error is when the the call the IFS_WRITE happens.

SQL_TO_CSV has a variable named file_content, which is a clob type, but IFS_WRITE has a varchar(32k) type. It works when the clob data is small enough for the varchar, but after 32k bytes, then it causes this error.

@worksofliam worksofliam marked this pull request as draft May 7, 2024 15:16
@worksofliam
Copy link
Contributor Author

Niels provided me with an updated IFS_WRITE that supports the clob parameter. I need to update the version used here to use that instead.

https://gist.github.com/NielsLiisberg/cd2350aee85f5b2e967993faf7ea7595

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 May 10, 2024 14:15
@worksofliam
Copy link
Contributor Author

@sebjulliand @chrjorgensen

This PR is now open again and ready to be tested. Some questions:

  • SQL_TO_CSV and IFS_WRITE were added to this PR, but it is not using them. I hate to say it, but CPYTOSTMF is significantly faster than SQL_TO_CSV, as proven by the test suite (specifically the 'getMemberList (advanced filtering)' test). Do you think we should just remove those component?

@worksofliam worksofliam added the ready Ready for review label May 10, 2024
@worksofliam worksofliam mentioned this pull request May 10, 2024
7 tasks
@chrjorgensen
Copy link
Collaborator

@worksofliam Didn't we start using pure SQL to get the line numbers and line dates from members, which CPYTOSTMF does not provide?

If people using these are punished performance-wize, so be it. Could be a motivator for moving to streamfiles and git! 😉

@worksofliam
Copy link
Contributor Author

@chrjorgensen

which CPYTOSTMF does not provide?

Right, but we only use CPYTOIMPF on temporary objects in QTEMP!

@edmundreinhardt edmundreinhardt self-requested a review May 13, 2024 17:44
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
@edmundreinhardt
Copy link
Collaborator

@sebjulliand @chrjorgensen

This PR is now open again and ready to be tested. Some questions:

  • SQL_TO_CSV and IFS_WRITE were added to this PR, but it is not using them. I hate to say it, but CPYTOSTMF is significantly faster than SQL_TO_CSV, as proven by the test suite (specifically the 'getMemberList (advanced filtering)' test). Do you think we should just remove those component?

I had a bit of chat with Liam, the potential use cases are all best handled by the DB2 for i extension.
It probably increases the attack surface and we can get it back easily enough, so I recommend deleting them

@worksofliam
Copy link
Contributor Author

@sebjulliand I have removed SQL_TO_CSV and IFS_WRITE from this PR seeing as they were completely disabled anyway. Tests are still passing as expected (other than the one about tabs!)

(Whoops, I ran the tests without a workspace open!!)

image

Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
Signed-off-by: Seb Julliand <sebjulliand@gmail.com>
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.

It works fine. I fixed a test that failed because Assert.fail didn't seem to be the right option.
I used the extension normally and it looks OK; I didn't see any issue.

@worksofliam, you can merge if you feel comfortable with it.

@worksofliam worksofliam merged commit 1057520 into master May 14, 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
4 participants