Add relationshipLink to ElideNavigator for JSON:API relationship endpoints#286
Add relationshipLink to ElideNavigator for JSON:API relationship endpoints#286Brutus5000 wants to merge 3 commits into
Conversation
…oints
navigateRelationship addresses the related resource (/data/{type}/{id}/{name}),
but JSON:API also defines a relationship-linkage endpoint
(/data/{type}/{id}/relationships/{name}) for reading/modifying the linkage itself
(PATCH/POST/DELETE of members). Add relationshipLink(name) on ElideNavigatorOnId
to build that path so callers don't hand-build it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a terminal relationship-link navigator for JSON:API, with a new interface, API method, route construction, include-order validation, and tests. ChangesRelationship Link Navigation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.25.0)api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.java[ERROR] Cannot load ruleset .coderabbit-pmd-ruleset.xml: Cannot resolve rule/ruleset reference '.coderabbit-pmd-ruleset.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. api/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnId.java[ERROR] Cannot load ruleset .coderabbit-pmd-ruleset.xml: Cannot resolve rule/ruleset reference '.coderabbit-pmd-ruleset.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. api/src/test/java/com/faforever/commons/api/elide/ElideNavigatorTest.java[ERROR] Cannot load ruleset .coderabbit-pmd-ruleset.xml: Cannot resolve rule/ruleset reference '.coderabbit-pmd-ruleset.xml'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Previously relationshipLink set a field on the navigator and returned itself, so it could coexist with a navigated relationship and be mutated further, allowing nonsensical paths. It now returns a dedicated terminal type (ElideNavigatorOnRelationshipLink) that only exposes build(), builds the path eagerly, and rejects includes on the parent (mirroring navigateRelationship). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.java`:
- Around line 118-123: The relationshipLink(`@NotNull` String name) method
currently allows a null name to be concatenated into the route, so add an
explicit null check before build() is used and fail fast like
navigateRelationship(...). Keep the existing includes guard and log.trace, but
ensure relationshipLink rejects null before constructing the "/relationships/"
path so the `@NotNull` contract is enforced consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9243ccf-aff3-477c-b3b9-a649e5bf7129
📒 Files selected for processing (4)
api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.javaapi/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnId.javaapi/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnRelationshipLink.javaapi/src/test/java/com/faforever/commons/api/elide/ElideNavigatorTest.java
| public ElideNavigatorOnRelationshipLink relationshipLink(@NotNull String name) { | ||
| if (!includes.isEmpty()) { | ||
| throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent"); | ||
| } | ||
| log.trace("relationship link added: {}", name); | ||
| String route = build() + "/relationships/" + name; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject null relationship names before path concatenation.
relationshipLink(null) currently builds .../relationships/null because String concatenation coerces null. That weakens the @NotNull contract and diverges from navigateRelationship(...), which fails fast. Add an explicit null check before building the route.
Proposed fix
`@Override`
public ElideNavigatorOnRelationshipLink relationshipLink(`@NotNull` String name) {
if (!includes.isEmpty()) {
throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent");
}
+ java.util.Objects.requireNonNull(name, "name");
log.trace("relationship link added: {}", name);
String route = build() + "/relationships/" + name;
return () -> route;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ElideNavigatorOnRelationshipLink relationshipLink(@NotNull String name) { | |
| if (!includes.isEmpty()) { | |
| throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent"); | |
| } | |
| log.trace("relationship link added: {}", name); | |
| String route = build() + "/relationships/" + name; | |
| public ElideNavigatorOnRelationshipLink relationshipLink(`@NotNull` String name) { | |
| if (!includes.isEmpty()) { | |
| throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent"); | |
| } | |
| java.util.Objects.requireNonNull(name, "name"); | |
| log.trace("relationship link added: {}", name); | |
| String route = build() + "/relationships/" + name; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.java` around
lines 118 - 123, The relationshipLink(`@NotNull` String name) method currently
allows a null name to be concatenated into the route, so add an explicit null
check before build() is used and fail fast like navigateRelationship(...). Keep
the existing includes guard and log.trace, but ensure relationshipLink rejects
null before constructing the "/relationships/" path so the `@NotNull` contract is
enforced consistently.
Mirror navigateRelationship by taking the related entity class: relationshipLink(Class<R>, name) now returns ElideNavigatorOnRelationshipLink<R>, which exposes getDtoClass() so callers can derive the related type (e.g. to build the linkage body) instead of passing it separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnId.java (1)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the terminal-operation Javadoc to mention
getDtoClass().The comment still says the returned navigator only exposes
build(), but this PR makes the related DTO type part of the public contract as well. Please documentgetDtoClass()here so the API description matches the implementation and tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnId.java` around lines 15 - 20, The terminal-operation Javadoc on ElideNavigatorOnId should be updated to reflect the new public contract: it currently says the returned navigator only allows ElideNavigatorOnRelationshipLink#build(), but it should also mention getDtoClass(). Update the documentation for relationshipLink(Class<R>, String) so it explicitly states that the returned navigator supports both build() and getDtoClass(), keeping the API comment aligned with the implementation and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.java`:
- Around line 118-133: The relationshipLink method in ElideNavigator should fail
fast when entityClass is null instead of returning a terminal navigator with a
null DTO class. Add a null check for the entityClass parameter before creating
the anonymous ElideNavigatorOnRelationshipLink and make it throw immediately, so
getDtoClass() is never backed by a null value.
---
Nitpick comments:
In `@api/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnId.java`:
- Around line 15-20: The terminal-operation Javadoc on ElideNavigatorOnId should
be updated to reflect the new public contract: it currently says the returned
navigator only allows ElideNavigatorOnRelationshipLink#build(), but it should
also mention getDtoClass(). Update the documentation for
relationshipLink(Class<R>, String) so it explicitly states that the returned
navigator supports both build() and getDtoClass(), keeping the API comment
aligned with the implementation and tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac6de138-f230-42b3-9355-01e97b52da9b
📒 Files selected for processing (4)
api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.javaapi/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnId.javaapi/src/main/java/com/faforever/commons/api/elide/ElideNavigatorOnRelationshipLink.javaapi/src/test/java/com/faforever/commons/api/elide/ElideNavigatorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/test/java/com/faforever/commons/api/elide/ElideNavigatorTest.java
| public <R extends ElideEntity> ElideNavigatorOnRelationshipLink<R> relationshipLink(@NotNull Class<R> entityClass, | ||
| @NotNull String name) { | ||
| if (!includes.isEmpty()) { | ||
| throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent"); | ||
| } | ||
| log.trace("relationship link added: {}", name); | ||
| String route = build() + "/relationships/" + name; | ||
| return new ElideNavigatorOnRelationshipLink<>() { | ||
| @Override | ||
| public String build() { | ||
| return route; | ||
| } | ||
|
|
||
| @Override | ||
| public Class<R> getDtoClass() { | ||
| return entityClass; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail fast on null entityClass.
The new terminal navigator makes getDtoClass() part of the contract. Right now relationshipLink(null, "mapVersion") still succeeds and returns a navigator with a null DTO class, so callers fail later when they derive linkage bodies from it. Validate entityClass before constructing the terminal navigator.
Proposed fix
public <R extends ElideEntity> ElideNavigatorOnRelationshipLink<R> relationshipLink(`@NotNull` Class<R> entityClass,
`@NotNull` String name) {
+ java.util.Objects.requireNonNull(entityClass, "entityClass");
if (!includes.isEmpty()) {
throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public <R extends ElideEntity> ElideNavigatorOnRelationshipLink<R> relationshipLink(@NotNull Class<R> entityClass, | |
| @NotNull String name) { | |
| if (!includes.isEmpty()) { | |
| throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent"); | |
| } | |
| log.trace("relationship link added: {}", name); | |
| String route = build() + "/relationships/" + name; | |
| return new ElideNavigatorOnRelationshipLink<>() { | |
| @Override | |
| public String build() { | |
| return route; | |
| } | |
| @Override | |
| public Class<R> getDtoClass() { | |
| return entityClass; | |
| public <R extends ElideEntity> ElideNavigatorOnRelationshipLink<R> relationshipLink(`@NotNull` Class<R> entityClass, | |
| `@NotNull` String name) { | |
| java.util.Objects.requireNonNull(entityClass, "entityClass"); | |
| if (!includes.isEmpty()) { | |
| throw new IllegalStateException("Cannot navigate to a relationship link with includes on parent"); | |
| } | |
| log.trace("relationship link added: {}", name); | |
| String route = build() + "/relationships/" + name; | |
| return new ElideNavigatorOnRelationshipLink<>() { | |
| `@Override` | |
| public String build() { | |
| return route; | |
| } | |
| `@Override` | |
| public Class<R> getDtoClass() { | |
| return entityClass; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/src/main/java/com/faforever/commons/api/elide/ElideNavigator.java` around
lines 118 - 133, The relationshipLink method in ElideNavigator should fail fast
when entityClass is null instead of returning a terminal navigator with a null
DTO class. Add a null check for the entityClass parameter before creating the
anonymous ElideNavigatorOnRelationshipLink and make it throw immediately, so
getDtoClass() is never backed by a null value.
1 participant
What
Adds
relationshipLink(name)toElideNavigatorOnId, which builds the JSON:API relationship-linkage endpoint:Why
navigateRelationship(Class, name)already exists, but it addresses the related resource (/data/{type}/{id}/{name}). JSON:API also defines a separate relationship endpoint (.../relationships/{name}) used to read or modify the linkage itself — e.g.PATCHto replace,POSTto add,DELETEto remove members of a relationship.Without this, callers that need to operate on a relationship link have to hand-build the path string (
"/data/" + type + "/" + id + "/relationships/" + name), which is exactly what surfaced in a downlords-faf-client review (clearingplayer.currentAvatar).Usage
It also composes after
navigateRelationship:Tests
Added
testRelationshipLinkandtestRelationshipLinkAfterNavigateRelationshiptoElideNavigatorTest(18 tests green).🤖 Generated with Claude Code
Summary by CodeRabbit