-
Notifications
You must be signed in to change notification settings - Fork 16
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!: Switch oslc:maxSize from int to BigInteger #272
Conversation
36a5b42
to
28c4ef7
Compare
Too little time to test before the 5.0 release. |
Kudos, SonarCloud Quality Gate passed! |
516d821
to
37f1833
Compare
3a6c05f
to
095037a
Compare
95114de
to
ff419fe
Compare
ff419fe
to
77099ff
Compare
3a8e589
to
4844aef
Compare
f59138f
to
eb97e39
Compare
eb97e39
to
0b7a57c
Compare
@Jad-el-khoury @jamsden I plan to merge this into 6.0.0-SNAPSHOT soon (today, ideally) and release it with 6.0.0 in October(ish). Should give us enough time to test. For the record, I was floating the idea to use a Long, which would allow to use up to 9,223,372,036,854,775,807 TRS Events but Frej voiced disagreement given that the OSLC TRS Spec requires support of arbitrary length integers. I agreed (mostly), it will be easier to have the main SDK support the spec fully. The performance hit is expected to be small, given that we don't do any arithmetic on those IDs. |
0b7a57c
to
6d17c27
Compare
sounds reasonable to me |
@jadelkhoury @jamsden I apologize for the confusion, I just went over the PR diff and it seems like a change to Lyo Core, not just to the TRS. More specifically, https://docs.oasis-open-projects.org/oslc-op/core/v3.0/os/core-vocab.html#maxSize See https://docs.oasis-open-projects.org/oslc-op/core/v3.0/os/core-shapes.html#PropertyShape for the shape, it demands an unlimited |
Now that I read the shape definition and https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#length(), I am not so sure this PR is a great idea (though not immediately against it).
try {
if (getSomeValue().length() > getMaxSize().intValueExact()) {
new OslcError(":someValue string exceeds oslc:maxSize");
}
} catch (ArithmeticException e) {
new OslcError("oslc:maxSize=%s exceeds max JVM String length".formatted(getMaxSize()));
} instead of just doing if (getSomeValue().length() > getMaxSize()) {
new OslcError(":someValue string exceeds oslc:maxSize");
} The only benefit to this this approach is to be able to catch the |
The TRS |
Given the purpose of maxSize, it there is little motivation to change to BigInteger then. Just close this PR? |
The problem is that right now Lyo is non-conformant to the OSLC Core spec (2.0+), which requires a valid implementation to accept |
I see. Maybe we the "Literal Value Types" need to be extended in the specs to also include xsd:int or xsd:long. How about we wait until a decision is settled on oslc-op/oslc-specs#595, before moving on this PR? |
6d17c27
to
6d36a64
Compare
6d36a64
to
036b7a6
Compare
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
036b7a6
to
751f1a6
Compare
Quality Gate passedIssues Measures |
1 similar comment
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
Pulled from my old notes:
|
Description
Switch oslc:maxSize from int to BigInteger as per spec.
Checklist
Issues
Closes #0; Closes #00
(use exactly this syntax to link to issues, one issue per statement only!)