-
Notifications
You must be signed in to change notification settings - Fork 29
Adds Duration Extension and Offset Function support #331
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
Adds Duration Extension and Offset Function support #331
Conversation
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java
Outdated
Show resolved
Hide resolved
CedarJava/src/main/java/com/cedarpolicy/serializer/ValueDeserializer.java
Show resolved
Hide resolved
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
| if (isNegative) { | ||
| totalMs = Math.negateExact(totalMs); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code before this is going to overflow before negating the i64::MIN value. In other words, this code should be able to parse duration("-9223372036854775808ms") but it doesn't currently because it'll overflow before getting here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I'll update it and also add the test cases from lean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in the revision
| "^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.)" | ||
| + "{3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$"); | ||
| + "{3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)" | ||
| + "(?:/(?:[0-9]|[12][0-9]|3[0-2]))?$"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean you couldn't accept ranges before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently yes. We just caught it due to the updated corpus tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, this fix probably deserves its own PR. You could probably ship it faster that way, and also include unit tests to make sure we've actually fixed this case. Not a blocker for this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a couple of tests in a subsequent PR
…ases Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Signed-off-by: Mudit Chaudhary <chmudit@amazon.com>
Overview
Changes
com.cedarpolicy.value.Durationcom.cedarpolicy.value.functions.Offsetequalsorhashas this is supposed to represent a functioncom.cedarpolicy.value.Decimalcom.cedarpolicy.value.IpAddressExample Usage
Duration
Offset