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

fix!: Switch oslc:maxSize from int to BigInteger #272

Closed
wants to merge 2 commits into from

Conversation

berezovskyi
Copy link
Contributor

@berezovskyi berezovskyi commented May 5, 2022

Description

Switch oslc:maxSize from int to BigInteger as per spec.

Checklist

  • This PR adds an entry to the CHANGELOG. See https://keepachangelog.com/en/1.0.0/ for instructions. Minor edits are exempt.
  • This PR was tested on at least one Lyo OSLC server or adds unit/integration tests.
  • This PR does NOT break the API

Issues

Closes #0; Closes #00

(use exactly this syntax to link to issues, one issue per statement only!)

@berezovskyi
Copy link
Contributor Author

Too little time to test before the 5.0 release.

@berezovskyi berezovskyi added this to the 6.0 milestone May 9, 2022
@sonarcloud
Copy link

sonarcloud bot commented May 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@github-actions github-actions bot force-pushed the b-property_maxsz branch 2 times, most recently from 3a6c05f to 095037a Compare April 27, 2023 18:26
@berezovskyi berezovskyi changed the title Switch oslc:maxSize from int to BigInteger fix!: Switch oslc:maxSize from int to BigInteger Apr 27, 2023
@github-actions github-actions bot force-pushed the b-property_maxsz branch 2 times, most recently from 95114de to ff419fe Compare April 27, 2023 23:46
@github-actions github-actions bot force-pushed the b-property_maxsz branch 2 times, most recently from 3a8e589 to 4844aef Compare July 14, 2023 07:43
@github-actions github-actions bot force-pushed the b-property_maxsz branch 2 times, most recently from f59138f to eb97e39 Compare August 5, 2023 07:43
@berezovskyi
Copy link
Contributor Author

berezovskyi commented Aug 11, 2023

@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.

@jadelkhoury
Copy link
Contributor

sounds reasonable to me

@berezovskyi
Copy link
Contributor Author

@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 xsd:integer instead of xsd:int, just as I wrote.

@berezovskyi
Copy link
Contributor Author

berezovskyi commented Aug 12, 2023

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).

oslc:maxSize property was designed to limit string length of RDF properties. In Java, https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/String.html#length() is an int32. What is important is that int and long are comparable directly in Java, but BigInteger cannot be directly compared to int or long. This means that to use that field for its original purpose, we'd always have to do:

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 ArithmeticException cleanly when downcasting BigInteger to an int32 and produce a corresponding OSLC Error resource.

@berezovskyi
Copy link
Contributor Author

The TRS BigInteger change was made in Lyo 5.0: https://github.com/eclipse/lyo/blob/master/CHANGELOG.md#changed-2

@Jad-el-khoury
Copy link
Contributor

Given the purpose of maxSize, it there is little motivation to change to BigInteger then. Just close this PR?

@berezovskyi
Copy link
Contributor Author

berezovskyi commented Aug 12, 2023

The problem is that right now Lyo is non-conformant to the OSLC Core spec (2.0+), which requires a valid implementation to accept xsd:integer, which in Java requires a BigInteger. But I agree on the motivation (or, rather, lack of it).

@Jad-el-khoury
Copy link
Contributor

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?

Copy link

sonarcloud bot commented May 23, 2024

1 similar comment
Copy link

sonarcloud bot commented Jun 22, 2024

Copy link

sonarcloud bot commented Sep 19, 2024

@berezovskyi
Copy link
Contributor Author

Pulled from my old notes:

  1. Keep oslc:maxSize as in the POJO, because both in the spec and Lyo it semantically means the length of the string and in many systems string length is an int (for sure in Java and C#).
  2. Keep oslc_trs:order a BigInteger in the POJO, to match the spec and especially to support cases like the use of UUIDv7 as TRS event identifiers / order numbers.
  3. Generate oslc_trs:order using an AtomicLong in TRS Server. Rationale: will exceed the limit with 5B events/s for 50y. Also, easy to replace later with Redis INCR or Hazelcast IAtomicLong implementation later for the distributed case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants