Skip to content

data: option to allow json int/bool as strings #2344

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 17, 2025
Merged

data: option to allow json int/bool as strings #2344

merged 4 commits into from
Feb 17, 2025

Conversation

bradh352
Copy link
Contributor

@bradh352 bradh352 commented Feb 6, 2025 •
edited
Loading

Prior to v1.0.212 the default behavior was to allow numbers
and boolean values to be in quotes, which is technically a
violation of the spec.

This adds a new LYD_PARSE_JSON_STRING_DATATYPES parse option
which will restore the prior behavior when enabled.

SONiC is using v1.0.73 currently and has a large installed base which
may be in violation of the new behavior, so adding such a flag is
required for this usecase.

Signed-off-by: Brad House (@bradh352)

@bradh352 bradh352 changed the title data: option to loosen data type validation data: option to loosen json data type validation Feb 6, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 7, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
ad6e284 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 8, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
974c595 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 9, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
0cb185e 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 9, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
abdd449 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 11, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
683f17a 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 11, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
8fcd3fd 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 12, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
04fc41b 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 13, 2025
@bradh352
libyang3: patch needed to loosen datatype validation
6932f9c 
libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.

Sent upstream here:
CESNET/libyang#2344
@michalvasko
Copy link
Member

Not sure if a project not working correctly justifies supporting such invalid use-cases but I suppose adding a flag does not break anything. It just seems it should be a JSON parsing flag instead of a context flag.

@bradh352
Copy link
Contributor Author

bradh352 commented Feb 14, 2025 •
edited
Loading

Not sure if a project not working correctly justifies supporting such invalid use-cases but I suppose adding a flag does not break anything. It just seems it should be a JSON parsing flag instead of a context flag.

Yeah, I wasn't around when the initial implementation was done, but since libyang wasn't validating in v1 I guess assumptions were made.

Regarding it being a parser option, Ok, sure, it wasn't obvious to me how the code path was setting flags, but I see now that the code is casting between lyd_ctx and lyjson_ctx (which wasn't immediately obvious), so I would be able to pass down the flags there.

So would this work? :

  • Create a new LYD_PARSE_JSON_LOOSE_DATATYPES flag
  • Create a new LYD_VALHINT_JSON_LOOSE_DATATYPES flag
  • Modify lydjson_value_type_hint() to also take lyd_json_ctx
  • In lydjson_value_type_hint() if lyd_json_ctx->parse_opts & LYD_PARSE_JSON_LOOSE_DATATYPES set additional hint of LYD_VALHINT_JSON_LOOSE_DATATYPES
  • In lyplg_type_check_hints() use the new hint to determine if strict type enforcement is on

If that would be acceptable to you then I'll rework this patch.

I'd really like to solidify the plan that will be accepted here since we're working on upstreaming libyang3 in SONiC:
sonic-net/sonic-buildimage#21679

@michalvasko
Copy link
Member

I do not think you really need LYD_VALHINT_JSON_LOOSE_DATATYPES but rather something like LYPLG_TYPE_STORE_IGNORE_VALHINT to pass this information to plugin types. LYD_PARSE_JSON_LOOSE_DATATYPES should be fine.

@bradh352
Copy link
Contributor Author

I do not think you really need LYD_VALHINT_JSON_LOOSE_DATATYPES but rather something like LYPLG_TYPE_STORE_IGNORE_VALHINT to pass this information to plugin types. LYD_PARSE_JSON_LOOSE_DATATYPES should be fine.

I don't know if completely ignoring the valhint is good, I was leaving in place validation of if a string value wasn't enclosed in quotes since I haven't seen an instance of that, and truthfully I didn't check if v1 allowed that or not. But I could go either way as I don't know if it would cause issues if we disabled that validation too ... the main issues that I see all over the place is SONiC expects "1234" == 1234, and "true" == true

@bradh352 bradh352 force-pushed the loose_datatypes branch 2 times, most recently from 887edf1 to 885d013 Compare February 14, 2025 14:01
@michalvasko
Copy link
Member

Fine, then call it accordingly. Something like LYD_PARSE_JSON_STRING_DATATYPES and LYPLG_TYPE_STORE_ALLOW_STRING_VALHINT.

@bradh352 bradh352 force-pushed the loose_datatypes branch 2 times, most recently from b05580c to 4933baf Compare February 14, 2025 14:27
@bradh352
Copy link
Contributor Author

Fine, then call it accordingly. Something like LYD_PARSE_JSON_STRING_DATATYPES and LYPLG_TYPE_STORE_ALLOW_STRING_VALHINT.

please look at the PR now, I reworked it. i can rename the options/flags whatever you want, just want to make sure this looks like I'm on the right track. I added test cases too.

@bradh352
Copy link
Contributor Author

bradh352 commented Feb 14, 2025 •
edited
Loading

also I added lydctx to some functions but the jsonctx was already there ... i'm assuming its probably better to extract the jsonctx from the lydctx in those functions? I couldn't tell but it looked like the jsonctx might get manipulated, but didn't check to see if the base pointer would ever change, and wasn't sure of the implications of that. just let me know if I should instead be replacing jsonctx in those functions with lydctx and then doing lydctx->jsonctx when the jsonctx is needed.

@michalvasko
Copy link
Member

Of course, you only need lydctx, since jsonctx is a member. Also, please change the base branch to devel, we do not accept PRs into master. If you then rename the values, it should be fine.

@bradh352 bradh352 changed the base branch from master to devel February 14, 2025 15:29
@bradh352
data: option to allow json int/bool as strings
9ba71bf 
Prior to v1.0.212 the default behavior was to allow numbers
and boolean values to be in quotes, which is technically a
violation of the spec.

This adds a new `LYD_PARSE_JSON_STRING_DATATYPES` parse option
which will restore the prior behavior when enabled.

SONiC is using v1.0.73 currently and has a large installed base which
may be in violation of the new behavior, so adding such a flag is
required for this usecase.

Signed-off-by: Brad House <brad@brad-house.com>
@bradh352
Copy link
Contributor Author

@michalvasko

Of course, you only need lydctx, since jsonctx is a member. Also, please change the base branch to devel, we do not accept PRs into master. If you then rename the values, it should be fine.

done with all your suggestions...

@bradh352 bradh352 changed the title data: option to loosen json data type validation data: option to allow json int/bool as strings Feb 14, 2025
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Feb 16, 2025
@bradh352
data: option to allow json int/bool as strings
ca54ed6 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
@bradh352 bradh352 mentioned this pull request Feb 16, 2025
Open
@bradh352 bradh352 mentioned this pull request Feb 23, 2025
Open
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
@bradh352
libyang3: patch needed to allow int/boolean as json strings
ccfb036 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
@bradh352
libyang3: patch needed to allow int/boolean as json strings
ca25bc0 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
@bradh352
libyang3: patch needed to allow int/boolean as json strings
892380f 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 28, 2025
@bradh352
libyang3: patch needed to allow int/boolean as json strings
7b0bead 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 1, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
2184724 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
@bradh352 bradh352 deleted the loose_datatypes branch March 3, 2025 11:11
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 4, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
12fbe42 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 5, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
b757810 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 5, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
c7c224b 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 6, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
872bc74 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 8, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
68ecfc0 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 11, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
8f5fb2c 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 12, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
969badc 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Apr 3, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
c53fd8f 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Apr 3, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
74f2d31 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Apr 20, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
0543339 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Apr 22, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
d862cc8 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Apr 23, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
b07bb23 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Apr 24, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
3782494 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Jun 13, 2025
@bradh352
libyang3: patch: needed to allow int/boolean as json strings
05ca53c 
CESNET/libyang#2344

libyang1 would treat "true" and true as identical, same with "1234" and 1234.
We need to restore this behavior so we don't break users.
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jun 13, 2025
@bradh352
data: option to allow json int/bool as strings
a5a2698 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jun 13, 2025
@bradh352
data: option to allow json int/bool as strings
55cf6b0 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
@bradh352 bradh352 mentioned this pull request Jun 20, 2025
Open
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jun 29, 2025
@bradh352
data: option to allow json int/bool as strings
a212ca0 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jun 29, 2025
@bradh352
data: option to allow json int/bool as strings
c127f99 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jul 6, 2025
@bradh352
data: option to allow json int/bool as strings
e5c241c 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jul 6, 2025
@bradh352
data: option to allow json int/bool as strings
810f744 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jul 6, 2025
@bradh352
data: option to allow json int/bool as strings
e3467e6 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
bradh352 added a commit to bradh352/libyang-python that referenced this pull request Jul 6, 2025
@bradh352
data: option to allow json int/bool as strings
c7831a7 
Depends on CESNET/libyang#2344

Add support for new option to python bindings.

Signed-off-by: Brad House <brad@brad-house.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalvasko michalvasko michalvasko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bradh352 @michalvasko