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

fix(Byte): update isHex validator #543

Merged
merged 2 commits into from
Nov 23, 2020
Merged

fix(Byte): update isHex validator #543

merged 2 commits into from
Nov 23, 2020

Conversation

rufman
Copy link
Contributor

@rufman rufman commented Oct 19, 2020

The base64 and hex validators are not guaranteeed to work.
There are some false positive cases, so instead add a type header to
the string, so a user can specify what the encoding of the string is.

The base64 and hex validators are not guaranteeed to work.
There are some false positive cases, so instead add a type header to
the string, so a user can specify what the encoding of the string is.
@rufman
Copy link
Contributor Author

rufman commented Nov 13, 2020

Is there anything else that is needed for this PR to be mergeable?

@ardatan
Copy link
Collaborator

ardatan commented Nov 14, 2020

Isn't that a breaking change?

@rufman
Copy link
Contributor Author

rufman commented Nov 16, 2020

Well it changes the functionality, yes. But the previous parsing didn't work in subtle ways.

@ardatan
Copy link
Collaborator

ardatan commented Nov 16, 2020

Instead of breaking the existing ones, we can have a better unit tests or two different scalars like one for Hex and another one for Base64. What do you think?

@rufman
Copy link
Contributor Author

rufman commented Nov 16, 2020

Yeah I can create 2 new Byte scalars. But would we deprecate Byte? Since determining if something is a byte or hex string isn't always 100% correct.

There were cases where the hex regex validator would return true
for a value that was acutally base64 encoded. Added tests for this
case and updated the validator.
@rufman
Copy link
Contributor Author

rufman commented Nov 18, 2020

I was able to fix the hex validator, which would sometime return true for base64 values.

@rufman rufman changed the title fix(Byte): replace validators with explicit type fix(Byte): update isHex validator Nov 18, 2020
@ardatan
Copy link
Collaborator

ardatan commented Nov 19, 2020

atob and btoa can be used for Base64 validation, no?

@ardatan ardatan merged commit e0600de into Urigo:master Nov 23, 2020
@MichalLytek
Copy link

@rufman Something is wrong with the hexValidator function:

const buf = Buffer.from([4, 8, 15, 16, 23, 42]);
const hex = buf.toString("hex");
console.log(hex); // 04080f10172a
console.log(hexValidator(hex)); // false

@rufman
Copy link
Contributor Author

rufman commented Feb 17, 2021

Hmm leading 0 is the issue.

fix is here: #717

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.

3 participants