-
Notifications
You must be signed in to change notification settings - Fork 18
fix(trs): consume RefImpl and Jazz TRS feeds #873
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
base: master
Are you sure you want to change the base?
Conversation
this could happen if a server starts with an empty base (legit, afaik) and keeps adding change events to the log, never rebasing on the server side (legit, afaik) Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
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.
Pull request overview
This pull request fixes a bug in the TRS (Tracked Resource Set) client where a nil base cutoff event was incorrectly treated as a reason to trigger a server rollback exception. The fix enables proper handling of servers that start with an empty base and accumulate change events without performing server-side rebases.
Changes:
- Refactored the
fetchRemoteChangeLogsmethod to properly handle null and RDF.nil cutoff events - Changed from a do-while loop to a while loop with clearer control flow
- Added upfront checks to determine if all change events should be processed when there's no valid cutoff event
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private boolean fetchRemoteChangeLogs(ChangeLog currentChangeLog, List<ChangeLog> changeLogs) { | ||
| boolean foundChangeEvent = false; | ||
| URI previousChangeLog; | ||
| do { | ||
| if (currentChangeLog != null) { | ||
| changeLogs.add(currentChangeLog); | ||
| if (lastProcessedChangeEventUri == null || RDF.nil.getURI().equals(lastProcessedChangeEventUri.toString())) { | ||
| foundChangeEvent = true; | ||
| } | ||
|
|
||
| while (currentChangeLog != null) { | ||
| changeLogs.add(currentChangeLog); | ||
| if (lastProcessedChangeEventUri != null && !RDF.nil.getURI().equals(lastProcessedChangeEventUri.toString())) { | ||
| if (ProviderUtil.changeLogContainsEvent(lastProcessedChangeEventUri, | ||
| currentChangeLog)) { | ||
| foundChangeEvent = true; | ||
| break; | ||
| } | ||
| previousChangeLog = currentChangeLog.getPrevious(); | ||
| currentChangeLog = trsClient.fetchRemoteChangeLog(previousChangeLog); | ||
| } else { | ||
| } | ||
|
|
||
| URI previousChangeLog = currentChangeLog.getPrevious(); | ||
| if (previousChangeLog == null || RDF.nil.getURI().equals(previousChangeLog.toString())) { | ||
| break; | ||
| } | ||
| } while (!RDF.nil.getURI().equals(previousChangeLog.toString())); | ||
|
|
||
| currentChangeLog = trsClient.fetchRemoteChangeLog(previousChangeLog); | ||
| } | ||
| return foundChangeEvent; | ||
| } |
Copilot
AI
Jan 16, 2026
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.
This fix addresses an important scenario where a server starts with an empty base (cutoff event is null or RDF.nil) and adds change events without rebasing. However, there doesn't appear to be test coverage for this specific scenario. Consider adding a test case that verifies the behavior when lastProcessedChangeEventUri is set to RDF.nil (from an empty base) to ensure this fix works correctly and to prevent regression.
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
trs/client/trs-client/src/main/java/org/eclipse/lyo/trs/client/handlers/TrsProviderHandler.java
Outdated
Show resolved
Hide resolved
trs/client/trs-client/src/main/java/org/eclipse/lyo/trs/client/handlers/TrsProviderHandler.java
Outdated
Show resolved
Hide resolved
trs/client/trs-client/src/main/java/org/eclipse/lyo/trs/client/handlers/TrsProviderHandler.java
Outdated
Show resolved
Hide resolved
| if (ProviderUtil.isNilUri(lastProcessedChangeEventUri) && ProviderUtil.isNilUri(previousChangeLog)) { | ||
| foundChangeEvent = true; | ||
| break; | ||
| } |
Copilot
AI
Jan 23, 2026
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 new nil-handling branch in fetchRemoteChangeLogs changes rollback/rebase behavior when both the stored cutoff and the changelog previous link are rdf:nil (or null via isNilUri). There doesn't appear to be a unit test covering this path for TrsProviderHandler (existing tests set cutoffEvent to a non-nil URI). Please add/extend a test that exercises the lastProcessedChangeEventUri == rdf:nil + currentChangeLog.previous == rdf:nil case to prevent regressions.
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.
we already do this for the concurrent provider in ConcurrentTrsProviderHandlerTest - we should unify the logic if possible
...s-client/src/main/java/org/eclipse/lyo/trs/client/handlers/ConcurrentTrsProviderHandler.java
Outdated
Show resolved
Hide resolved
trs/client/trs-client/src/main/java/org/eclipse/lyo/trs/client/handlers/TrsProviderHandler.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-junit-jupiter</artifactId> | ||
| <version>5.21.0</version> |
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.
should be dep-managed
| <dependency> | ||
| <groupId>junit</groupId> | ||
| <artifactId>junit</artifactId> | ||
| <version>4.13.2</version> |
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.
why?
Mainly due to DirectContainer, the current approach is quite hacky Signed-off-by: Andrew Berezovskyi <andriib@kth.se>
| throws TrsEndpointConfigException, TrsEndpointErrorException, LyoModelException { | ||
| final Response.StatusType responseInfo = response.getStatusInfo(); | ||
| final Response.Status.Family httpCodeType = responseInfo.getFamily(); | ||
| log.info("HTTP response status: {} {}", responseInfo.getStatusCode(), responseInfo.getReasonPhrase()); |
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.
debug
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.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Resource containerResource = directContainers.next(); | ||
| log.debug("Found ldp:DirectContainer at {}", containerResource.getURI()); | ||
|
|
||
| Base base = new Base(); | ||
| try { | ||
| base.setAbout(new URI(containerResource.getURI())); | ||
| } catch (URISyntaxException e) { | ||
| throw new LyoModelException(e); | ||
| } | ||
|
|
||
| // Extract cutoffEvent | ||
| if (containerResource.hasProperty(rdFModel.getProperty(TRS_CUTOFF_EVENT))) { | ||
| Resource cutoffResource = containerResource.getPropertyResourceValue( | ||
| rdFModel.getProperty(TRS_CUTOFF_EVENT) | ||
| ); | ||
| if (cutoffResource != null && cutoffResource.isURIResource()) { | ||
| try { | ||
| base.setCutoffEvent(new URI(cutoffResource.getURI())); | ||
| } catch (URISyntaxException e) { | ||
| throw new LyoModelException(e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract members (try both rdfs:member and ldp:member) | ||
| org.apache.jena.rdf.model.Property memberProp = rdFModel.getProperty(RDFS_MEMBER); | ||
| if (!containerResource.hasProperty(memberProp)) { | ||
| memberProp = rdFModel.getProperty(LDP_MEMBER); | ||
| } | ||
|
|
||
| var memberStmts = containerResource.listProperties(memberProp); | ||
| while (memberStmts.hasNext()) { | ||
| var stmt = memberStmts.next(); | ||
| if (stmt.getObject().isURIResource()) { | ||
| try { | ||
| base.getMembers().add(new URI(stmt.getObject().asResource().getURI())); | ||
| } catch (URISyntaxException e) { | ||
| log.warn("Invalid member URI: {}", stmt.getObject()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| log.debug("Extracted Base with {} members from DirectContainer", base.getMembers().size()); | ||
| return base; |
Copilot
AI
Jan 26, 2026
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.
containerResource.getURI() can be null if the ldp:DirectContainer subject is a blank node; this will cause a NullPointerException when constructing new URI(...). Consider skipping non-URI resources (e.g., check isURIResource() / getURI()!=null) and continuing to the next match, or returning null with a clear error.
| Resource containerResource = directContainers.next(); | |
| log.debug("Found ldp:DirectContainer at {}", containerResource.getURI()); | |
| Base base = new Base(); | |
| try { | |
| base.setAbout(new URI(containerResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| // Extract cutoffEvent | |
| if (containerResource.hasProperty(rdFModel.getProperty(TRS_CUTOFF_EVENT))) { | |
| Resource cutoffResource = containerResource.getPropertyResourceValue( | |
| rdFModel.getProperty(TRS_CUTOFF_EVENT) | |
| ); | |
| if (cutoffResource != null && cutoffResource.isURIResource()) { | |
| try { | |
| base.setCutoffEvent(new URI(cutoffResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| } | |
| } | |
| // Extract members (try both rdfs:member and ldp:member) | |
| org.apache.jena.rdf.model.Property memberProp = rdFModel.getProperty(RDFS_MEMBER); | |
| if (!containerResource.hasProperty(memberProp)) { | |
| memberProp = rdFModel.getProperty(LDP_MEMBER); | |
| } | |
| var memberStmts = containerResource.listProperties(memberProp); | |
| while (memberStmts.hasNext()) { | |
| var stmt = memberStmts.next(); | |
| if (stmt.getObject().isURIResource()) { | |
| try { | |
| base.getMembers().add(new URI(stmt.getObject().asResource().getURI())); | |
| } catch (URISyntaxException e) { | |
| log.warn("Invalid member URI: {}", stmt.getObject()); | |
| } | |
| } | |
| } | |
| log.debug("Extracted Base with {} members from DirectContainer", base.getMembers().size()); | |
| return base; | |
| while (directContainers.hasNext()) { | |
| Resource containerResource = directContainers.next(); | |
| // Skip DirectContainer subjects that are not URI resources (e.g., blank nodes) | |
| if (!containerResource.isURIResource() || containerResource.getURI() == null) { | |
| log.debug("Skipping non-URI ldp:DirectContainer resource: {}", containerResource); | |
| continue; | |
| } | |
| log.debug("Found ldp:DirectContainer at {}", containerResource.getURI()); | |
| Base base = new Base(); | |
| try { | |
| base.setAbout(new URI(containerResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| // Extract cutoffEvent | |
| if (containerResource.hasProperty(rdFModel.getProperty(TRS_CUTOFF_EVENT))) { | |
| Resource cutoffResource = containerResource.getPropertyResourceValue( | |
| rdFModel.getProperty(TRS_CUTOFF_EVENT) | |
| ); | |
| if (cutoffResource != null && cutoffResource.isURIResource()) { | |
| try { | |
| base.setCutoffEvent(new URI(cutoffResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| } | |
| } | |
| // Extract members (try both rdfs:member and ldp:member) | |
| org.apache.jena.rdf.model.Property memberProp = rdFModel.getProperty(RDFS_MEMBER); | |
| if (!containerResource.hasProperty(memberProp)) { | |
| memberProp = rdFModel.getProperty(LDP_MEMBER); | |
| } | |
| var memberStmts = containerResource.listProperties(memberProp); | |
| while (memberStmts.hasNext()) { | |
| var stmt = memberStmts.next(); | |
| if (stmt.getObject().isURIResource()) { | |
| try { | |
| base.getMembers().add(new URI(stmt.getObject().asResource().getURI())); | |
| } catch (URISyntaxException e) { | |
| log.warn("Invalid member URI: {}", stmt.getObject()); | |
| } | |
| } | |
| } | |
| log.debug("Extracted Base with {} members from DirectContainer", base.getMembers().size()); | |
| return base; | |
| } | |
| log.warn("No ldp:DirectContainer with a URI subject found in RDF model"); | |
| return null; |
| if (!directContainers.hasNext()) { | ||
| return null; | ||
| } | ||
|
|
||
| Resource containerResource = directContainers.next(); | ||
| log.debug("Found ldp:DirectContainer at {}", containerResource.getURI()); | ||
|
|
||
| Base base = new Base(); | ||
| try { | ||
| base.setAbout(new URI(containerResource.getURI())); | ||
| } catch (URISyntaxException e) { | ||
| throw new LyoModelException(e); | ||
| } | ||
|
|
||
| // Extract cutoffEvent | ||
| if (containerResource.hasProperty(rdFModel.getProperty(TRS_CUTOFF_EVENT))) { | ||
| Resource cutoffResource = containerResource.getPropertyResourceValue( | ||
| rdFModel.getProperty(TRS_CUTOFF_EVENT) | ||
| ); | ||
| if (cutoffResource != null && cutoffResource.isURIResource()) { | ||
| try { | ||
| base.setCutoffEvent(new URI(cutoffResource.getURI())); | ||
| } catch (URISyntaxException e) { | ||
| throw new LyoModelException(e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract members (try both rdfs:member and ldp:member) | ||
| org.apache.jena.rdf.model.Property memberProp = rdFModel.getProperty(RDFS_MEMBER); | ||
| if (!containerResource.hasProperty(memberProp)) { | ||
| memberProp = rdFModel.getProperty(LDP_MEMBER); | ||
| } | ||
|
|
||
| var memberStmts = containerResource.listProperties(memberProp); | ||
| while (memberStmts.hasNext()) { | ||
| var stmt = memberStmts.next(); | ||
| if (stmt.getObject().isURIResource()) { | ||
| try { | ||
| base.getMembers().add(new URI(stmt.getObject().asResource().getURI())); | ||
| } catch (URISyntaxException e) { | ||
| log.warn("Invalid member URI: {}", stmt.getObject()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| log.debug("Extracted Base with {} members from DirectContainer", base.getMembers().size()); | ||
| return base; |
Copilot
AI
Jan 26, 2026
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.
ResIterator/statement iterators from Jena should be closed to avoid resource leaks for some Model implementations. Consider using try/finally (or try-with-resources where applicable) to close directContainers and the memberStmts iterator.
| if (!directContainers.hasNext()) { | |
| return null; | |
| } | |
| Resource containerResource = directContainers.next(); | |
| log.debug("Found ldp:DirectContainer at {}", containerResource.getURI()); | |
| Base base = new Base(); | |
| try { | |
| base.setAbout(new URI(containerResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| // Extract cutoffEvent | |
| if (containerResource.hasProperty(rdFModel.getProperty(TRS_CUTOFF_EVENT))) { | |
| Resource cutoffResource = containerResource.getPropertyResourceValue( | |
| rdFModel.getProperty(TRS_CUTOFF_EVENT) | |
| ); | |
| if (cutoffResource != null && cutoffResource.isURIResource()) { | |
| try { | |
| base.setCutoffEvent(new URI(cutoffResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| } | |
| } | |
| // Extract members (try both rdfs:member and ldp:member) | |
| org.apache.jena.rdf.model.Property memberProp = rdFModel.getProperty(RDFS_MEMBER); | |
| if (!containerResource.hasProperty(memberProp)) { | |
| memberProp = rdFModel.getProperty(LDP_MEMBER); | |
| } | |
| var memberStmts = containerResource.listProperties(memberProp); | |
| while (memberStmts.hasNext()) { | |
| var stmt = memberStmts.next(); | |
| if (stmt.getObject().isURIResource()) { | |
| try { | |
| base.getMembers().add(new URI(stmt.getObject().asResource().getURI())); | |
| } catch (URISyntaxException e) { | |
| log.warn("Invalid member URI: {}", stmt.getObject()); | |
| } | |
| } | |
| } | |
| log.debug("Extracted Base with {} members from DirectContainer", base.getMembers().size()); | |
| return base; | |
| try { | |
| if (!directContainers.hasNext()) { | |
| return null; | |
| } | |
| Resource containerResource = directContainers.next(); | |
| log.debug("Found ldp:DirectContainer at {}", containerResource.getURI()); | |
| Base base = new Base(); | |
| try { | |
| base.setAbout(new URI(containerResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| // Extract cutoffEvent | |
| if (containerResource.hasProperty(rdFModel.getProperty(TRS_CUTOFF_EVENT))) { | |
| Resource cutoffResource = containerResource.getPropertyResourceValue( | |
| rdFModel.getProperty(TRS_CUTOFF_EVENT) | |
| ); | |
| if (cutoffResource != null && cutoffResource.isURIResource()) { | |
| try { | |
| base.setCutoffEvent(new URI(cutoffResource.getURI())); | |
| } catch (URISyntaxException e) { | |
| throw new LyoModelException(e); | |
| } | |
| } | |
| } | |
| // Extract members (try both rdfs:member and ldp:member) | |
| org.apache.jena.rdf.model.Property memberProp = rdFModel.getProperty(RDFS_MEMBER); | |
| if (!containerResource.hasProperty(memberProp)) { | |
| memberProp = rdFModel.getProperty(LDP_MEMBER); | |
| } | |
| var memberStmts = containerResource.listProperties(memberProp); | |
| try { | |
| while (memberStmts.hasNext()) { | |
| var stmt = memberStmts.next(); | |
| if (stmt.getObject().isURIResource()) { | |
| try { | |
| base.getMembers().add(new URI(stmt.getObject().asResource().getURI())); | |
| } catch (URISyntaxException e) { | |
| log.warn("Invalid member URI: {}", stmt.getObject()); | |
| } | |
| } | |
| } | |
| } finally { | |
| memberStmts.close(); | |
| } | |
| log.debug("Extracted Base with {} members from DirectContainer", base.getMembers().size()); | |
| return base; | |
| } finally { | |
| directContainers.close(); | |
| } |
| private String getFixtureBody(String fixtureName) throws IOException { | ||
| Path path = Path.of("src/test/resources/fixtures", fixtureName); | ||
| return Files.readString(path).replace("http://localhost:8080", baseUri); | ||
| } |
Copilot
AI
Jan 26, 2026
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.
This test reads fixtures via Path.of("src/test/resources/..."), which depends on the working directory and can be flaky in IDEs/build tools. Prefer loading fixtures from the classpath (e.g., getResourceAsStream) to make the test environment-independent.
| final Response.StatusType responseInfo = response.getStatusInfo(); | ||
| final Response.Status.Family httpCodeType = responseInfo.getFamily(); | ||
| log.info("HTTP response status: {} {}", responseInfo.getStatusCode(), responseInfo.getReasonPhrase()); | ||
| log.info("HTTP response headers: {}", response.getHeaders()); |
Copilot
AI
Jan 26, 2026
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.
Logging full HTTP response headers at INFO can leak sensitive data (e.g., Authorization/Cookie) and will be very noisy in production. Consider moving this to DEBUG/TRACE and/or redacting sensitive headers before logging.
| log.info("HTTP response headers: {}", response.getHeaders()); | |
| log.debug("HTTP response headers: {}", response.getHeaders()); |
For RefImpl:
Do not treat a nil base cutoff as a rebase reason if it has been fetched before and had the same
rdf:nilvalue.This could happen if a server starts with an empty base (legit, afaik) and keeps adding change events to the log, never rebasing on the server side (legit, afaik). The PR prevents an endless rebase loop that continues until the server base becomes non-empty.
For Jazz:
Handle ldp:DirectContainer, not just ldp:Container.
The handling is quite hacky and should be refactored.
Checklist
@oslc-bot /test-allif not sure) or adds unit/integration tests.mvn package org.openrewrite.maven:rewrite-maven-plugin:run spotless:apply -DskipTests -P'!enforcer'if not, commit & push)