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

CBOR validation for type choice in array #116

Closed
tomachristian opened this issue Apr 15, 2022 · 7 comments · Fixed by #121
Closed

CBOR validation for type choice in array #116

tomachristian opened this issue Apr 15, 2022 · 7 comments · Fixed by #121
Assignees
Labels
bug Something isn't working validation
Milestone

Comments

@tomachristian
Copy link

tomachristian commented Apr 15, 2022

Using the latest version at this point in time: 0.9.0-beta.1.

I am having a problem with validating CBOR [18] (hex: 9F12FF - regardless of definite/indefinite array) against CDDL tester = [18/12] which should be valid.

But I am getting:

failed: error validating type choice at cbor location : expected value 12, got Array([Integer(Integer(18))])

and validating CBOR [12] (hex: 9F0CFF) against the same CDDL also yields:

failed: error validating type choice at cbor location /0: expected value 18, got Integer(12)
error validating type choice at cbor location : expected value 12, got Array([Integer(Integer(12))])

but validating CBOR [12] and [18] against CDDL tester = [18//12] works.

According to the specification type choices should have worked, am I mistaken or is this something that is currently not supported in this package?

Thank you for your great work!

@anweiss
Copy link
Owner

anweiss commented Apr 15, 2022

Thanks for reporting @tomachristian! Indeed this is a bug as I have repro'd on my end. Will take a look at this.

@anweiss anweiss self-assigned this Apr 15, 2022
@anweiss anweiss added bug Something isn't working validation labels Apr 15, 2022
@anweiss anweiss added this to the v1.0.0 milestone Apr 15, 2022
@anweiss
Copy link
Owner

anweiss commented Apr 15, 2022

@tomachristian fixed in #117 and will be included in the 0.9.0 release.

@tomachristian
Copy link
Author

Thank you @anweiss! I will have a try on it. I have quite a large protocol described with lots of CDDL and I am currently using the "official" gem for validating CDDL (the one mentioned from the spec), but it has lots of bugs and is outdated... I really really want to use your validator instead, so I will have another try with it, would you mind if I come back with other bugs? :)

@anweiss
Copy link
Owner

anweiss commented Apr 15, 2022

Sounds great!! Yes, no problem at all. The more feedback and bug reports, the better. I've been maintaining this crate myself for a few years now and only recently has there been more traction with CDDL, and thus, this crate has garnered more interest from folks in the community.

@tomachristian
Copy link
Author

CBOR is getting traction, so I think that is why CDDL is recently getting the same as well

@tomachristian
Copy link
Author

tomachristian commented Apr 16, 2022

Hello @anweiss!

I checked the fix and it works for the specific scenario outlined above, but there are still problems with CDDL

tester = [$vals]
$vals /= 18
$vals /= 12

(Also tried without tester=[$vals], just with tester=$vals to same effect (of course I updated the CBOR as well by stripping out the array begin/end)

against the same input CBOR as above. The error message is similar to the previous one:

failed: error validating at cbor location : expected type $vals, got Integer(Integer(18))

So I would say that this problem still exists with a similar effect even though the cause here might be different.

PS: If you'd like, you can write some smoke tests and I can provide a lot of such scenarios (that would also protect the software from regressions).

@anweiss
Copy link
Owner

anweiss commented Apr 18, 2022

Also a bug. Will fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants