codefori / vscode-ibmi

🌍 IBM i development extension for VS Code
https://codefori.github.io/docs/#/
MIT License
288 stars 96 forks source link

Wrong characters in object browser using QCCSID 37 #2088

Open SanjulaGanepola opened 6 months ago

SanjulaGanepola commented 6 months ago

After creating a new connection for the first time and connecting, I noticed that the NL symbols are not rendering correctly in the object browser. However, after disconnecting and reconnecting, they appear fine. I was able to reproduce this on a system where the QCCSID was 37, but not reproducible if the QCCSID was 65535 (explained further below as to why this may be the case).

Result after first connect: image

Result after disconnecting and reconnecting: image

After trying to debug this issue, I noticed a few things:

Context Version
Code for IBM i version 2.10.4
Visual Studio Code version 1.89.1
Operating System win32_x64
Active extensions ``` Code for IBM i Walkthroughs (vscode-ibmi-walkthroughs): 0.5.0 Db2 for IBM i (vscode-db2i): 1.0.0 Emmet (emmet): 1.0.0 Git (git): 1.0.0 Git Base (git-base): 1.0.0 GitHub (github): 0.0.1 GitHub Authentication (github-authentication): 0.0.2 IBM i Debug (ibmidebug): 1.0.0 IBM i Project Explorer (vscode-ibmi-projectexplorer): 2.10.5 JSON Language Features (json-language-features): 1.0.0 Merge Conflict (merge-conflict): 1.0.0 Node Debug Auto-attach (debug-auto-launch): 1.0.0 TODO Highlight (vscode-todo-highlight): 1.0.5 TypeScript and JavaScript Language Features (typescript-language-features): 1.0.0 WCA@IBM (wca-at-ibm): 6.3.0 WSL (remote-wsl): 0.88.2 ```

Remote system |Setting|Value| |-|-| |IBM i OS|n/a| |Tech Refresh|n/a| |CCSID Origin|37| |Runtime CCSID|933| |Default CCSID|933| |SQL|Enabled |Source dates|Disabled ### Enabled features |/QOpenSys/pkgs/bin|/usr/bin|/QSYS.lib/ILEDITOR.lib|/QSYS.LIB|/QIBM/ProdData/IBMiDebugService/bin| |-|-|-|-|-| |bash|attr|GETNEWLIBL.PGM|QZDFMDB2.PGM|startDebugService.sh| |chsh|iconv|||| |git|setccsid|||| |grep||||| |ls||||| |md5sum||||| |sort||||| |stat||||| |tar||||| |tn5250|||||
Shell env ```bash BUILDLIB=AURORAKOR CURLIB=AURORAKOR HOME=/home/AURORAKOR HOST=p8adt05.rch.stglabs.ibm.com LIBLS=QTEMP QGPL TESTTOOLS LOGIN=aurorakor LOGNAME=aurorakor MAIL=/var/spool/mail/aurorakor OLDPWD=/home/AURORAKOR PATH=/QOpenSys/pkgs/bin:/QOpenSys/usr/bin:/usr/ccs/bin:/QOpenSys/usr/bin/X11:/usr/sbin:.:/usr/bin PWD=/home/AURORAKOR SHELL=/QOpenSys/pkgs/bin/bash SHLVL=1 SSH_CLIENT=9.61.105.209 7985 22 SSH_CONNECTION=9.61.105.209 7985 9.5.12.241 22 TZ=0 USER=aurorakor USERNAME=AURORAKOR WORKDIR=/home/AURORAKOR _=/QOpenSys/pkgs/bin/env ```
Variants ```json { "american": "#@$", "local": "#@$" } ```
Errors ```json [ { "command": "/QOpenSys/usr/bin/qsh", "code": 1, "stderr": "CPF9801: Object QCPTOIMPF in library QSYS not found.", "cwd": "/home/AURORAKOR" }, { "command": "/QOpenSys/usr/bin/qsh", "code": 1, "stderr": "CPF9801: Object QCPFRMIMPF in library QSYS not found.", "cwd": "/home/AURORAKOR" }, { "command": "ls undefined", "code": 2, "stderr": "ls: cannot access 'undefined': No such file or directory", "cwd": "/home/AURORAKOR" } ] ```
chrjorgensen commented 6 months ago

@SanjulaGanepola Good finds! I've seen this as well and it is a regression from when the CCSID handling was changed.

About the cached server settings being used for a manually deleted connection: I think we should include a checksum of the connection values in the cached server settings and remove it from cache if the checksums doesn't match.

worksofliam commented 6 months ago

@SanjulaGanepola

I believe that the original issue is caused by the fact that this block of code is only run if the QCCSID is 65535.

In theory, the block that is run only needs to happen when the QCCSID is invalid. When QCCSID is valid, then really the system should return the correct characters, though perhaps there might be a case (like when an old SSHD is running) where this might not be the case. Perhaps a test case we can run on two systems to determine this would be good.

I created a PR with the fix for the connection issue and cached data.

SanjulaGanepola commented 6 months ago

Perhaps a test case we can run on two systems to determine this would be good.

@worksofliam In terms of manually testing, with the branch for https://github.com/codefori/vscode-ibmi/pull/2089, you can consistently reproduce the issue on every connect now as long as you make sure to Connect and Reload Server Settings on a system were the QCCSID is 37. In terms of a test case, I believe that if the issues mentioned in https://github.com/codefori/vscode-ibmi/pull/2068 are fixed, it can be used to verify this as well because I merged https://github.com/codefori/vscode-ibmi/pull/2089 into that PR.

About the cached server settings being used for a manually deleted connection: I think we should include a checksum of the connection values in the cached server settings and remove it from cache if the checksums doesn't match.

@chrjorgensen An idea I had in mind was to simply setup a configuration listener on code-for-ibmi.connections and code-for-ibmi.connectionSettings that will get the stored keys in storage and remove any that no longer exists in the settings.json. I can take a look at adding that. Any issues with this approach?

SanjulaGanepola commented 6 months ago

In theory, the block that is run only needs to happen when the QCCSID is invalid.

@worksofliam I just took a look at the query that gets run after the change in your branch and in ACS I can see that the result it gets back is not correct.

image

WITH SRCPF AS (
         SELECT t.SYSTEM_TABLE_NAME AS NAME,
                '*FILE' AS TYPE,
                'PF' AS ATTRIBUTE,
                t.TABLE_TEXT AS TEXT,
                1 AS IS_SOURCE,
                t.ROW_LENGTH AS SOURCE_LENGTH
             FROM QSYS2.SYSTABLES AS t
             WHERE t.table_schema = 'AURORAKOR'
                   AND t.file_type = 'S'
     ),
     OBJD AS (
         SELECT OBJNAME AS NAME,
                OBJTYPE AS TYPE,
                OBJATTRIBUTE AS ATTRIBUTE,
                OBJTEXT AS TEXT,
                0 AS IS_SOURCE,
                IASP_NUMBER AS IASP_NUMBER,
                OBJSIZE AS SIZE,
                EXTRACT(EPOCH FROM (OBJCREATED)) * 1000 AS CREATED,
                EXTRACT(EPOCH FROM (CHANGE_TIMESTAMP)) * 1000 AS CHANGED,
                OBJOWNER AS OWNER,
                OBJDEFINER AS CREATED_BY
             FROM TABLE (
                     QSYS2.OBJECT_STATISTICS(OBJECT_SCHEMA => 'AURORAKOR ', OBJTYPELIST => '*ALL')
                 )
     )
    SELECT o.NAME,
           o.TYPE,
           o.ATTRIBUTE,
           o.TEXT,
           CASE
               WHEN s.IS_SOURCE IS NOT null THEN s.IS_SOURCE
               ELSE o.IS_SOURCE
           END AS IS_SOURCE,
           s.SOURCE_LENGTH,
           o.IASP_NUMBER,
           o.SIZE,
           o.CREATED,
           o.CHANGED,
           o.OWNER,
           o.CREATED_BY
        FROM OBJD o
             LEFT JOIN SRCPF s
                 ON o.NAME = s.NAME;
chrjorgensen commented 6 months ago

@chrjorgensen An idea I had in mind was to simply setup a configuration listener on code-for-ibmi.connections and code-for-ibmi.connectionSettings that will get the stored keys in storage and remove any that no longer exists in the settings.json. I can take a look at adding that. Any issues with this approach?

@SanjulaGanepola I didn't know about the configuration listeners - but this is an excellent idea. Go ahead - I'll be looking forward to see your solution. :smiley:

sebjulliand commented 6 months ago

@SanjulaGanepola good idea! 👍🏻

You may want to use this method: https://github.com/codefori/vscode-ibmi/blob/47b76278e1c3106fd64ba8f10dc6e42dc3286eea/src/api/Configuration.ts#L13

And check if these listeners here should be extended or if you'll need to add a call in a new place: https://github.com/codefori/vscode-ibmi/blob/47b76278e1c3106fd64ba8f10dc6e42dc3286eea/src/extension.ts#L82 https://github.com/codefori/vscode-ibmi/blob/47b76278e1c3106fd64ba8f10dc6e42dc3286eea/src/extension.ts#L83

worksofliam commented 6 months ago

@SanjulaGanepola

and in ACS I can see that the result it gets back is not correct.

This must be related to profile or system CCSID setup. I ran this with AURORAKOR before (when working with Jonathan) and it worked fine.

Just for reference, check out this page which mentions about how object names are stored:

The characters in names are converted to CCSID 37 when the names are stored. Quoted names, however, are stored using the CCSID of the job.

SanjulaGanepola commented 6 months ago

This must be related to profile or system CCSID setup. I ran this with AURORAKOR before (when working with Jonathan) and it worked fine.

@worksofliam I tested on 3 different machines and I am still not getting back the right symbols. I tried with the following:

worksofliam commented 6 months ago

@SanjulaGanepola So, we've got the SQL statement to get a list of objects, but can also share an SQL statement (or CL command) to create those objects with the expected name and text?

worksofliam commented 6 months ago

@SanjulaGanepola I guess your test cases in #2068 can create those objects.

SanjulaGanepola commented 6 months ago

I guess your test cases in #2068 can create those objects.

Yes I used the test cases to create those objects. Here are also the commands I used:

system 'CRTLIB LIB("Aܧ#$%Ä") TEXT("§#$öße")'
system 'CRTSRCPF FILE("Aܧ#$%Ä"/"àáãÄ£ø") RCDLEN(112) CCSID(273) TEXT("Üä$öß")'
system 'ADDPFM FILE("Aܧ#$%Ä"/"àáãÄ£ø") MBR("§#$MAN") SRCTYPE(CBLLE) TEXT("§#$öße")'