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

Enabling in regorus the arbitrary_precision feature by default on serde_json can impact projects that depend on regorus. #199

Closed
lquerel opened this issue Apr 9, 2024 · 7 comments · Fixed by #203

Comments

@lquerel
Copy link

lquerel commented Apr 9, 2024

A project using serde_json without the arbitrary_precision feature enabled, which then starts depending on regorus, will see this feature automatically propagate throughout their entire project. This can have an impact, or not, on the project in question due to the change in behavior associated with this specific feature.

This is particularly the case for me, as the deserialization of untagged enums interacts quite poorly with this feature. As a workaround, I had to fork regorus to remove this feature to make my project work. I encourage the maintainers of this project to make this feature optional in regorus.

Otherwise, I want to say that this project is fantastic, and I am very seriously considering using regorus within the OpenTelemetry Weaver project.

@anakrish
Copy link
Collaborator

@lquerel
Thanks for reporting this issue. The goal initially was to get the OPA test suite to pass and hence by default, big float suport was enabled. However, the semantics of arbitrary precision in OPA/Rego is not that well defined. This is something that we'd like to improve in the language definition. Also, big floats are computationally more expensive than regular precision.

I've created #202 to default to regular precision and also to complete a few improvements around number support.

@anakrish
Copy link
Collaborator

@lquerel
Great to hear about the use of regorus in weaver project.

I happened to take a look at the linked PR and noticed the use of eval_query. I'd recommend using eval_rule (which is available in main and will be available in next release) if possible. It is more performant and is a better fit if OPA style QueryResults are not needed.

See #186 (comment)

@lquerel
Copy link
Author

lquerel commented Apr 10, 2024

Thank you @anakrish !
I'm not sure what the timeline implications are for the other fixes mentioned in #202, but would it be possible to have a release just for the removal of the feature on serde_json so that I can remove the reference to my fork from the Weaver project as soon as possible?

@lquerel
Copy link
Author

lquerel commented Apr 10, 2024

@lquerel

Great to hear about the use of regorus in weaver project.

I happened to take a look at the linked PR and noticed the use of eval_query. I'd recommend using eval_rule (which is available in main and will be available in next release) if possible. It is more performant and is a better fit if OPA style QueryResults are not needed.

See #186 (comment)

I will make this change. Thanks for the advice.

Another reason to have a new regorus release :-)

@anakrish
Copy link
Collaborator

Thank you @anakrish ! I'm not sure what the timeline implications are for the other fixes mentioned in #202, but would it be possible to have a release just for the removal of the feature on serde_json so that I can remove the reference to my fork from the Weaver project as soon as possible?

Sure. Let me see how best to make that happen. It will likely be next week since I'm tasked on other things for the remainder of this week.

anakrish added a commit that referenced this issue Apr 11, 2024
The feature does not interoperate well with other serde_json features like untagged enums.

Fixes #199

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
@anakrish
Copy link
Collaborator

@lquerel I have just pushed a new release :)

@lquerel
Copy link
Author

lquerel commented Apr 11, 2024

@anakrish Thank you!

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 a pull request may close this issue.

2 participants