vscode-ibmi icon indicating copy to clipboard operation
vscode-ibmi copied to clipboard

Component API

Open worksofliam opened this issue 10 months ago • 12 comments

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:

  • #1993 for when QCCSID is valid
  • #1992 being implemented to solve some CCSID issues when fetching the object list

To do

  • [x] Install components if not installed
  • [x] Check the state of a component (installed or not)
  • [x] Improve version control of individual components
  • [x] Added SQL_TO_CSV component
  • [x] Added IFS_WRITE component
  • [x] Inner component dependencies? (SQL_TO_CSV depends on IFS_WRITE)
  • [x] Fix handling of null values in SQL_TO_CSV
  • [ ] Write components to a new dedicated library if available
  • [x] Test cases for runSQL API using SQL_TO_CSV
  • [x] Encoding tests (steal from #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
  • [x] #1968 merged

How to test this PR

Examples:

  1. Run the test cases

Checklist

  • [x] 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 avatar Apr 11 '24 17:04 worksofliam

@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...'.

chrjorgensen avatar Apr 12 '24 14:04 chrjorgensen

@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 avatar Apr 12 '24 17:04 worksofliam

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

worksofliam avatar Apr 15 '24 17:04 worksofliam

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

worksofliam avatar Apr 15 '24 20:04 worksofliam

@sebjulliand Thanks for your review and commits! I take it you are okay with my implementation of the getComponent API?

worksofliam avatar Apr 17 '24 14:04 worksofliam

@chrjorgensen Any chance I can get your help correcting / adding the GRANT to each SQL component here after the create statements?

worksofliam avatar Apr 23 '24 12:04 worksofliam

@worksofliam Sure. Do we agree that all users should be able to replace the SQL components?

chrjorgensen avatar Apr 23 '24 14:04 chrjorgensen

@worksofliam I added GRTOBJAUT to all SQL components instead of GRANT... since GRANT does not give *OBJMGT required when replacing a function / procedure.

https://www.ibm.com/docs/en/i/7.5?topic=statements-create-function-sql-table

chrjorgensen avatar Apr 23 '24 18:04 chrjorgensen

We still have two failing tests which, until I have fixed, will hold this PR up:

image

This one less important:

image

worksofliam avatar May 02 '24 13:05 worksofliam

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 avatar May 02 '24 14:05 worksofliam

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 avatar May 02 '24 14:05 worksofliam

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

worksofliam avatar May 07 '24 19:05 worksofliam

@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 avatar May 10 '24 14:05 worksofliam

@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! 😉

chrjorgensen avatar May 10 '24 17:05 chrjorgensen

@chrjorgensen

which CPYTOSTMF does not provide?

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

worksofliam avatar May 10 '24 19:05 worksofliam

@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

edmundreinhardt avatar May 13 '24 18:05 edmundreinhardt

@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

worksofliam avatar May 13 '24 18:05 worksofliam