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

[(develop)] fixed parseString when it trimming zero in the beggining #1549

Merged
merged 4 commits into from Sep 21, 2023
Merged

[(develop)] fixed parseString when it trimming zero in the beggining #1549

merged 4 commits into from Sep 21, 2023

Conversation

DarkFisk
Copy link
Contributor

@DarkFisk DarkFisk commented Sep 18, 2023

Hello,
I have a problem when I add new elements to an array, and even when I provide schema validation that only strings are allowed, JsonEditor always removes 0 in front of the number, after some investigation I found that parseString is casting values like 0102031234 to number and removes zero in front and making 102031234, that is absolutely wrong. (this number is actually example of CPR number in Denmark, it is like your passport number)
Please, take into consideration my PR.

P.S. @josdejong great tool, thank you!

@josdejong
Copy link
Owner

Thanks for bringing this up Viacheslav.

I think to work out a solution for this we have to look a bit deeper. You're PR only checks for values with a single leading zero, like 0123, but if we make a solution for this we should also take care of multiple leading zeros like 000123.

When you open a JSON document and it contains a string like 0102031234 , it will be loaded with type "string" (not "auto"), and leading zeros will be maintained. This is not the case though wen creating a new value and pasting the value 0102031234 there. So I think we should address that logic. You can have a look at the setValue method to get an idea. What do you think?

On a side note: this issue has been addressed already in the successor of this library, svelte-jsoneditor.

@josdejong
Copy link
Owner

Thinking about this more I think it would be proper to solve this in parseString, since a number with leading zeros is not valid in JSON, so it has to be a string there.

Can you add unit tests to verify that 0.3 and 0e3 are still parsed as numbers? Those also start with a zero but are valid numeric values.

@DarkFisk
Copy link
Contributor Author

Thank you josdejong for quick response,
I added one more test to PR to show you that my fix will treat value as string type no matter how many zero is in front of value.

regarding setValue you mentioned, it call function _getType, and it inside also call buggy parseString method, and it returns for string 0102031234 number without zero in front, but _getType logic returns string type if value is number, and auto for string... I went to crazy house)

@DarkFisk
Copy link
Contributor Author

Thinking about this more I think it would be proper to solve this in parseString, since a number with leading zeros is not valid in JSON, so it has to be a string there.

Can you add unit tests to verify that 0.3 and 0e3 are still parsed as numbers? Those also start with a zero but are valid numeric values.

you are right, it is not so easy if such cases also need to be supported, will take a look if it possible to handle such cases

@josdejong
Copy link
Owner

I think it is only these two cases.

@DarkFisk
Copy link
Contributor Author

@josdejong could you please review one more time? think it would be the safest way, to not break any other cases

@josdejong
Copy link
Owner

Ah, this regex looks solid indeed, thanks 👌

I see the linter complains about the semicolon ;, can you npm run format to fix the linting issue? Besides that we can merge your fix.

@DarkFisk
Copy link
Contributor Author

@josdejong thank you. I fixed lint error. if it will be merged, just wandering, when are you planning to release new version?

@josdejong josdejong merged commit 77246fc into josdejong:develop Sep 21, 2023
5 checks passed
@josdejong
Copy link
Owner

I'm publish the fix right away, there aren't other pending fixes/features right now.

@josdejong
Copy link
Owner

Published now in v9.10.3, thanks again Viacheslav!

@DarkFisk
Copy link
Contributor Author

Published now in v9.10.3, thanks again Viacheslav!

thank you very much, it is awesome news

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants