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

Correctly handle source members with variant characters in their name #1940

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

sebjulliand
Copy link
Collaborator

Changes

Fixes #1935

Member names used to be uppercased regarless of the variant characters they may contain. Turns out that à is one of CCSID 297 (French) variant chars and it gets uppercased to À, which raises a member not found error.

This PR adds a new upperCaseName method to the IBMi class to correctly uppercase names, preservingthe variant chars case.

How to test this PR

  1. Set PASE_LANG environment variable to FR_FR
  2. Open a connection with a profile whose CCSID is 297
  3. Try to open a member with one or more à in its name

Checklist

  • have tested my change
  • have created one or more test cases

@sebjulliand sebjulliand added the bug A confirmed issue when something isn't working as intended label Mar 18, 2024
@sebjulliand sebjulliand self-assigned this Mar 18, 2024
@sebjulliand sebjulliand mentioned this pull request Mar 18, 2024
Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand I gave the PR a spin, but had no success... 😞

With or without SQL enabled, when I tried to open a member having à in the name, I got the following error:

billede

The member name is correct in the list:

billede

???

Does it work at your end? Could be my system setup...

@sebjulliand
Copy link
Collaborator Author

@chrjorgensen check out my comment here: #1935 (comment)

I'll add a commit to this PR with the copy to a temporary member (I managed to open the member, I just need to apply the same logic when saving). We'll see how it goes (I'll un-draft the PR then).

@sebjulliand sebjulliand marked this pull request as ready for review March 18, 2024 12:31
@sebjulliand
Copy link
Collaborator Author

@chrjorgensen you can give it another try; I managed to open and save my member with a à using this patch.

@chrjorgensen
Copy link
Collaborator

@sebjulliand I have not tried your workaround yet - but when I look at the code, it seems you're copying to temporary member in case of any variant characters in the name. Which would mean most non-US users will be hit by this workaround, even though they've had no problems so far. Correct?

If correct, wouldn't it be better to only do the workaround if they would have been hit by the à problem? By testing for change when uppercasing the variants:

if ( this.ibmi.variantChars.local !== this.ibmi.variantChars.local.toLocaleUpperCase() ) {

That way we run as before for the majority of the users - no change for them... What do you think?

@sebjulliand
Copy link
Collaborator Author

@chrjorgensen you're right, let's reduce the scope only to those who use dangerous variants 😅

@sebjulliand
Copy link
Collaborator Author

@chrjorgensen there you go

@chrjorgensen chrjorgensen self-requested a review March 18, 2024 22:17
Copy link
Collaborator

@chrjorgensen chrjorgensen left a comment

Choose a reason for hiding this comment

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

@sebjulliand I can confirm being able to open and edit and save members having à in the member name when I simulate being French (CCSID 297 etc) when running your PR. 👍

Approved - merge at will... 😃

@sebjulliand
Copy link
Collaborator Author

Thank you @chrjorgensen !

@sebjulliand sebjulliand merged commit 555563c into master Mar 18, 2024
1 check passed
@sebjulliand sebjulliand deleted the fix/upperCasedMemberName branch March 18, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A confirmed issue when something isn't working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sources with "à"
2 participants