Shared element#247
Open
SlimaneAmar wants to merge 3 commits intomainfrom
Open
Conversation
added 2 commits
May 4, 2026 09:32
…into shared_element
📝 WalkthroughWalkthroughA references relationship was added end-to-end: DB changelog, JPA entity, repository methods, DTOs, and service mapping; service creation/mapping and constructor call sites were updated to include references and eager-load them; tests and test utilities were adjusted for the new DTO/constructor shapes. ChangesReferences relationship and eager-loading
sequenceDiagram
participant Client as Client
participant Service as DirectoryService
participant Repo as DirectoryElementRepository
participant DB as Database
participant DTO as ElementAttributes/ReferenceAttributes
Client->>Service: GET getDirectoryElements(recursive=true)
Service->>Repo: collect descendant IDs
Repo-->>Service: list of IDs
Service->>Repo: findAllWithReferencesByIdIn(ids)
Repo->>DB: query elements JOIN references (entity-graph)
DB-->>Repo: entities with references
Repo-->>Service: hydrated entities
Service->>DTO: toElementAttributesWithReferences(entity)
DTO-->>Service: ElementAttributes (with references)
Service-->>Client: response
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java (1)
90-91: ⚡ Quick winAdd one test with non-empty references to cover the new feature path.
Current updates only validate constructor compatibility with
List.of(). Please add a case whereDirectoryElementEntitycontains at least one reference and assert it is propagated inElementAttributes.🤖 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 `@src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java` around lines 90 - 91, Add a new test case that constructs a DirectoryElementEntity with a non-empty references list (e.g., List.of(someReference)) and pass it through toElementAttributes, then call verifyElementAttributes to assert the references are propagated into the resulting ElementAttributes; update or add an invocation similar to the existing calls but using DirectoryElementEntity(..., List.of(reference)) and ensure verifyElementAttributes checks that ElementAttributes.getReferences() contains the provided reference (use existing symbols DirectoryElementEntity, toElementAttributes, verifyElementAttributes, ElementAttributes, ELEMENT_UUID).
🤖 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 `@src/main/java/org/gridsuite/directory/server/dto/ElementAttributes.java`:
- Around line 92-93: The shared factory overload toElementAttributes currently
calls the full factory with List.of() for the references parameter, forcing an
empty list; change the call in ElementAttributes (the single-argument overload
that forwards to toElementAttributes(elementUuid, elementName, elementType,
userId, ...)) so it passes null for references instead of List.of() (or use the
existing overload that omits references) so “not provided” remains null and only
code that intentionally populates references supplies a list.
- Around line 57-60: The toReferencesAttributes method assumes
entity.getReferences() is non-null and will throw if it is null; update
toReferencesAttributes to guard against a null references collection (from
DirectoryElementEntity.getReferences) by treating null as empty and returning an
empty List when appropriate before streaming—e.g., check for null or use
Optional/Collections.emptyList()—and then map
ReferenceEntity::toReferenceAttributes as before.
In
`@src/main/java/org/gridsuite/directory/server/repository/DirectoryElementEntity.java`:
- Around line 65-67: The references collection on DirectoryElementEntity is
marked FetchType.EAGER which forces loading all ReferenceEntity rows on every
DirectoryElementEntity fetch (e.g., getDirectoryElements); change the `@OneToMany`
fetch to FetchType.LAZY and update call sites (such as getDirectoryElements and
any read-only paths) to explicitly initialize or fetch references only when
needed (e.g., via JOIN FETCH queries or explicit repository methods). Also
consider converting the unidirectional mapping into a bidirectional one by
adding a `@ManyToOne` back-reference on ReferenceEntity and using mappedBy on
DirectoryElementEntity to avoid extra UPDATE statements on child inserts if
writes to references become frequent.
In
`@src/main/java/org/gridsuite/directory/server/repository/ReferenceEntity.java`:
- Around line 27-29: The ReferenceEntity class declares a UUID primary key field
id annotated with `@Id` but lacks `@GeneratedValue`, so persist operations will fail
unless callers manually set an id; update the id field in ReferenceEntity to use
a UUID generator by adding `@GeneratedValue` with a UUID generator (e.g.,
`@GeneratedValue`(generator="UUID") plus a matching `@GenericGenerator`(name="UUID",
strategy="org.hibernate.id.UUIDGenerator")) and ensure the column is
non-nullable/updatable=false as appropriate so new ReferenceEntity instances get
auto-generated UUIDs.
In `@src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml`:
- Line 11: Update the Liquibase XML so the element_id column is declared NOT
NULL: change the <column name="element_id" type="UUID"/> entry to include a
not-null constraint (e.g. add a nested <constraints nullable="false"/> or set
constraints nullable="false" on the <column>) in changelog_20260429T154156Z.xml
so the DB enforces non-null for the element_id FK managed by JPA.
In `@src/test/java/org/gridsuite/directory/server/DirectoryTest.java`:
- Line 88: The class-level `@DisableElasticsearch` on DirectoryTest is too broad
and disables ES for tests that need it; remove the class-level annotation and
instead apply `@DisableElasticsearch` only to the specific test methods that do
not require Elasticsearch (or move non-ES tests into a new class, e.g.,
DirectoryTestWithoutElasticsearch, leaving ES-dependent tests in DirectoryTest
without the annotation). Update the test method declarations to add or remove
`@DisableElasticsearch` accordingly so ES-centric tests run with Elasticsearch
enabled while only the truly non-ES tests remain disabled.
- Around line 284-287: The test testGetPathOfElementType01 is not exercising
path retrieval because calls to getPath/getAllDirectoryElementsStream and the
ordered-parent assertions are commented out and
SQLStatementCountValidator.reset() is asserted before the path call; restore the
test by uncommenting the path retrieval lines (getPath(elementUUID, "Doe")
and/or directoryService.getAllDirectoryElementsStream(rootDirUuid,
List.of(DIRECTORY), "user1")), re-enable the ordered-parent assertions that
follow, and move or re-evaluate the
SQLStatementCountValidator.reset()/zero-request assertion so it runs at the
correct point (reset before invoking the path call or assert counts after the
call) to ensure the endpoint is actually exercised in
testGetPathOfElementType01.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java`:
- Around line 90-91: Add a new test case that constructs a
DirectoryElementEntity with a non-empty references list (e.g.,
List.of(someReference)) and pass it through toElementAttributes, then call
verifyElementAttributes to assert the references are propagated into the
resulting ElementAttributes; update or add an invocation similar to the existing
calls but using DirectoryElementEntity(..., List.of(reference)) and ensure
verifyElementAttributes checks that ElementAttributes.getReferences() contains
the provided reference (use existing symbols DirectoryElementEntity,
toElementAttributes, verifyElementAttributes, ElementAttributes, ELEMENT_UUID).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41972f13-7e42-46f3-9d70-b96b0d68a66f
📒 Files selected for processing (11)
src/main/java/org/gridsuite/directory/server/DirectoryService.javasrc/main/java/org/gridsuite/directory/server/dto/ElementAttributes.javasrc/main/java/org/gridsuite/directory/server/dto/ReferenceAttributes.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementEntity.javasrc/main/java/org/gridsuite/directory/server/repository/ReferenceEntity.javasrc/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xmlsrc/main/resources/db/changelog/db.changelog-master.yamlsrc/test/java/org/gridsuite/directory/server/DirectoryTest.javasrc/test/java/org/gridsuite/directory/server/ElementAttributesTest.javasrc/test/java/org/gridsuite/directory/server/SupervisionTest.javasrc/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java
SlimaneAmar
force-pushed
the
shared_element
branch
2 times, most recently
from
1a75459 to
4739be0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/gridsuite/directory/server/DirectoryService.java (2)
272-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis recursive branch still returns DTOs without
references.Lines 272-278 preload references, but the fetched list is discarded, and the final mapper still calls
ElementAttributes.toElementAttributes(...), which omits the new field. So this branch pays for the extra query but returns the old payload shape.One way to wire the fetched entities through
- List<UUID> ids = descendents.stream().map(DirectoryElementEntity::getId).toList(); - directoryElementRepository.findAllWithReferencesByIdIn(ids); + List<UUID> ids = descendents.stream().map(DirectoryElementEntity::getId).toList(); + Map<UUID, DirectoryElementEntity> descendentsWithReferences = directoryElementRepository.findAllWithReferencesByIdIn(ids) + .stream() + .collect(Collectors.toMap(DirectoryElementEntity::getId, java.util.function.Function.identity())); return descendents .stream() + .map(e -> descendentsWithReferences.getOrDefault(e.getId(), e)) .filter(e -> (types.isEmpty() || types.contains(e.getType())) && permissionService.hasReadPermissions(userId, List.of(e.getId()))) - .map(ElementAttributes::toElementAttributes) + .map(ElementAttributes::toElementAttributesWithReferences) .toList();🤖 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 `@src/main/java/org/gridsuite/directory/server/DirectoryService.java` around lines 272 - 279, The preloaded entities from directoryElementRepository.findAllWithReferencesByIdIn(ids) are discarded and you still map the original descendents (which lack references) via ElementAttributes.toElementAttributes; change the branch in DirectoryService to capture the returned List<DirectoryElementEntity> (from findAllWithReferencesByIdIn), build a map by id, then iterate the original descendents order/filter (respecting types and permissionService.hasReadPermissions) but map each element by looking up the fully-loaded entity in that map and call a mapper that includes references (either a new ElementAttributes.fromEntityWithReferences(entity) or extend ElementAttributes.toElementAttributes to accept the loaded entity), so the returned DTOs contain the populated references field.
151-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick winElement creation currently drops incoming
references.
ElementAttributesnow carriesreferences, but this constructor call always writesList.of(). Any references sent on create are silently ignored, so the new table is never populated from the API path. Either map the DTO references here or reject non-empty input explicitly.🤖 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 `@src/main/java/org/gridsuite/directory/server/DirectoryService.java` around lines 151 - 160, The DirectoryElementEntity creation currently ignores incoming references by passing List.of() — update the DirectoryService code that constructs DirectoryElementEntity (the call assigning elementEntity) to either map elementAttributes.getReferences() into the entity's reference list before construction or validate and reject non-empty references; specifically, replace the hardcoded List.of() with a mapped list derived from elementAttributes.getReferences() (or add a guard that throws IllegalArgumentException when elementAttributes.getReferences() is non-empty) so incoming references are not silently dropped.
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java (1)
79-91: ⚡ Quick winPlease cover the new
referencesfield in this suite.These assertions still only validate the legacy fields, so a regression where references are dropped during mapping or serialization will pass unnoticed. A small case for
toElementAttributesWithReferences(...)plus one JSON assertion with a non-emptyreferenceslist would lock the new feature down.Also applies to: 117-120
🤖 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 `@src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java` around lines 79 - 91, Add assertions in testElementAttributesCreation to validate the new references field: call the mapping helper that includes references (e.g., toElementAttributesWithReferences(...)) with a non-empty references list and pass the result to verifyElementAttributes (or add a new verifyElementAttributesWithReferences) and add at least one JSON serialization assertion that the produced JSON contains the expected non-empty "references" array; ensure you update the existing toElementAttributes(...) usages around testElementAttributesCreation and the block at lines 117-120 to include at least one case that exercises mapping/serialization of references so regressions that drop references will be detected.
🤖 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 `@src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml`:
- Around line 21-22: The new FK constraint element_id_fk between
reference(element_id) and element(id) will block
DirectoryElementRepository.deleteAllById(...) bulk deletes because child rows in
reference are not removed first; either modify the Liquibase <changeSet
id="1777477329203-3"> to add onDelete="CASCADE" to the addForeignKeyConstraint
(so deleting element rows cascades to reference), or update the
DirectoryElementRepository.deleteAllById implementation to explicitly delete
reference rows for the given element IDs before deleting from element (e.g., add
a repository call to deleteByElementIdIn(ids) or run a SQL DELETE FROM reference
WHERE element_id IN (...) prior to the bulk element delete).
---
Outside diff comments:
In `@src/main/java/org/gridsuite/directory/server/DirectoryService.java`:
- Around line 272-279: The preloaded entities from
directoryElementRepository.findAllWithReferencesByIdIn(ids) are discarded and
you still map the original descendents (which lack references) via
ElementAttributes.toElementAttributes; change the branch in DirectoryService to
capture the returned List<DirectoryElementEntity> (from
findAllWithReferencesByIdIn), build a map by id, then iterate the original
descendents order/filter (respecting types and
permissionService.hasReadPermissions) but map each element by looking up the
fully-loaded entity in that map and call a mapper that includes references
(either a new ElementAttributes.fromEntityWithReferences(entity) or extend
ElementAttributes.toElementAttributes to accept the loaded entity), so the
returned DTOs contain the populated references field.
- Around line 151-160: The DirectoryElementEntity creation currently ignores
incoming references by passing List.of() — update the DirectoryService code that
constructs DirectoryElementEntity (the call assigning elementEntity) to either
map elementAttributes.getReferences() into the entity's reference list before
construction or validate and reject non-empty references; specifically, replace
the hardcoded List.of() with a mapped list derived from
elementAttributes.getReferences() (or add a guard that throws
IllegalArgumentException when elementAttributes.getReferences() is non-empty) so
incoming references are not silently dropped.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java`:
- Around line 79-91: Add assertions in testElementAttributesCreation to validate
the new references field: call the mapping helper that includes references
(e.g., toElementAttributesWithReferences(...)) with a non-empty references list
and pass the result to verifyElementAttributes (or add a new
verifyElementAttributesWithReferences) and add at least one JSON serialization
assertion that the produced JSON contains the expected non-empty "references"
array; ensure you update the existing toElementAttributes(...) usages around
testElementAttributesCreation and the block at lines 117-120 to include at least
one case that exercises mapping/serialization of references so regressions that
drop references will be detected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64039835-4edf-459f-bdd6-1e85fbf4cf8d
📒 Files selected for processing (10)
src/main/java/org/gridsuite/directory/server/DirectoryService.javasrc/main/java/org/gridsuite/directory/server/dto/ElementAttributes.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementEntity.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.javasrc/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xmlsrc/test/java/org/gridsuite/directory/server/DirectoryServiceTest.javasrc/test/java/org/gridsuite/directory/server/DirectoryTest.javasrc/test/java/org/gridsuite/directory/server/ElementAttributesTest.javasrc/test/java/org/gridsuite/directory/server/PermissionServiceTest.javasrc/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/gridsuite/directory/server/DirectoryServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/gridsuite/directory/server/DirectoryTest.java
Comment on lines
+21
to
+22
| <changeSet author="amarsli (generated)" id="1777477329203-3"> | ||
| <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true"/> |
There was a problem hiding this comment.
Batch element deletes will hit the new FK.
DirectoryElementRepository.deleteAllById(...) still deletes straight from element. Once an element has rows in reference, this constraint will reject that bulk delete because nothing removes child rows first. Add DB-level delete cascading here, or clear reference rows before the parent bulk-delete path runs.
Possible Liquibase fix
- <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true"/>
+ <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true" onDelete="CASCADE"/>📝 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.
Suggested change
| <changeSet author="amarsli (generated)" id="1777477329203-3"> | |
| <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true"/> | |
| <changeSet author="amarsli (generated)" id="1777477329203-3"> | |
| <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true" onDelete="CASCADE"/> |
🤖 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 `@src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml`
around lines 21 - 22, The new FK constraint element_id_fk between
reference(element_id) and element(id) will block
DirectoryElementRepository.deleteAllById(...) bulk deletes because child rows in
reference are not removed first; either modify the Liquibase <changeSet
id="1777477329203-3"> to add onDelete="CASCADE" to the addForeignKeyConstraint
(so deleting element rows cascades to reference), or update the
DirectoryElementRepository.deleteAllById implementation to explicitly delete
reference rows for the given element IDs before deleting from element (e.g., add
a repository call to deleteByElementIdIn(ids) or run a SQL DELETE FROM reference
WHERE element_id IN (...) prior to the bulk element delete).
4739be0 to
7d1fe6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/main/java/org/gridsuite/directory/server/DirectoryService.java`:
- Around line 271-275: The eager-load call
directoryElementRepository.findAllWithReferencesByIdIn(ids) currently has its
result discarded and descendents (from repositoryService.findAllDescendants) is
returned and later transformed with toElementAttributes(), which drops
references; replace that by capturing the result of
findAllWithReferencesByIdIn(ids) (assign it to descendents or a new variable)
and use those returned entities when returning the response, and change the
mapping call from toElementAttributes() to toElementAttributesWithReferences()
so the response includes the loaded references; ensure
repositoryService.findAllDescendants(...) is only used to compute ids, not the
final returned list.
In `@src/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java`:
- Line 29: The test fixture creates DirectoryElementEntity with two separate
Instant.now() calls which can differ; capture a single Instant once (e.g.,
Instant now = Instant.now()) in DirectoryTestUtils and use that single timestamp
for both the creation and modification args when constructing
DirectoryElementEntity (apply the same change to both occurrences where two
Instant.now() are passed).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eef0efc7-504b-4d7c-a81a-94e39d6dc953
📒 Files selected for processing (11)
src/main/java/org/gridsuite/directory/server/DirectoryService.javasrc/main/java/org/gridsuite/directory/server/dto/ElementAttributes.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementEntity.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.javasrc/main/java/org/gridsuite/directory/server/repository/ReferenceEntity.javasrc/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xmlsrc/test/java/org/gridsuite/directory/server/DirectoryServiceTest.javasrc/test/java/org/gridsuite/directory/server/DirectoryTest.javasrc/test/java/org/gridsuite/directory/server/ElementAttributesTest.javasrc/test/java/org/gridsuite/directory/server/PermissionServiceTest.javasrc/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml
Comment on lines
271
to
275
| List<DirectoryElementEntity> descendents = repositoryService.findAllDescendants(directoryUuid).stream().toList(); | ||
| // Need to load references for all descendents (no N+1 -> only one query) | ||
| List<UUID> ids = descendents.stream().map(DirectoryElementEntity::getId).toList(); | ||
| directoryElementRepository.findAllWithReferencesByIdIn(ids); | ||
| return descendents |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Check transaction boundaries around recursive loading path"
rg -n --type=java 'getDirectoryElements\(|@Transactional|findAllDescendants\(|findAllWithReferencesByIdIn\('
echo
echo "2) Check if OpenEntityManagerInView is explicitly disabled/enabled"
rg -n 'spring\.jpa\.open-in-view' -g '*application*.yml' -g '*application*.yaml' -g '*application*.properties'
echo
echo "3) Inspect ElementAttributes mapping path for references access"
rg -n --type=java 'toElementAttributesWithReferences|toElementAttributes\s*\(.*DirectoryElementEntity'Repository: gridsuite/directory-server
Length of output: 4932
🏁 Script executed:
sed -n '259,285p' src/main/java/org/gridsuite/directory/server/DirectoryService.javaRepository: gridsuite/directory-server
Length of output: 1589
🏁 Script executed:
rg -n 'toElementAttributes' src/main/java/org/gridsuite/directory/server/ -A 15 --type=java -m 1Repository: gridsuite/directory-server
Length of output: 6045
🏁 Script executed:
rg -n 'toReferencesAttributes' src/main/java/org/gridsuite/directory/server/ -B 2 -A 5 --type=javaRepository: gridsuite/directory-server
Length of output: 2023
🏁 Script executed:
rg -n 'findAllWithReferencesByIdIn' src/main/java/org/gridsuite/directory/server/ -B 3 -A 3 --type=javaRepository: gridsuite/directory-server
Length of output: 2146
🏁 Script executed:
rg -n 'ElementAttributes.*references' src/main/java/org/gridsuite/directory/server/ --type=java -iRepository: gridsuite/directory-server
Length of output: 257
🏁 Script executed:
rg -n 'getReferences|references' src/main/java/org/gridsuite/directory/server/dto/ElementAttributes.java --type=java -B 1 -A 1Repository: gridsuite/directory-server
Length of output: 354
🏁 Script executed:
cat -n src/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.java | grep -A 20 'interface DirectoryElementRepository'Repository: gridsuite/directory-server
Length of output: 1154
🏁 Script executed:
git log --oneline -n 20 -- src/main/java/org/gridsuite/directory/server/DirectoryService.javaRepository: gridsuite/directory-server
Length of output: 93
🏁 Script executed:
rg -n 'toElementAttributesWithReferences' src/main/java/org/gridsuite/directory/server/ -B 2 -A 2 --type=javaRepository: gridsuite/directory-server
Length of output: 702
🏁 Script executed:
git show 7d1fe6e --stat | head -20Repository: gridsuite/directory-server
Length of output: 1100
🏁 Script executed:
git show 7d1fe6e -- src/main/java/org/gridsuite/directory/server/DirectoryService.java | grep -A 30 'getDirectoryElements'Repository: gridsuite/directory-server
Length of output: 1792
🏁 Script executed:
sed -n '90,110p' src/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.javaRepository: gridsuite/directory-server
Length of output: 1124
🏁 Script executed:
cat -n src/main/java/org/gridsuite/directory/server/dto/ElementAttributes.java | sed -n '1,85p'Repository: gridsuite/directory-server
Length of output: 4009
Replace the discarded eager-load result with the eagerly-loaded entities to include references in the response.
At line 274, findAllWithReferencesByIdIn(ids) is called but its result is discarded; the method still maps descendents from the earlier query. Additionally, toElementAttributes() (used at line 280) ignores references entirely—leaving references always null. If references are intended in the response, use the eager-loaded entities and call toElementAttributesWithReferences() instead.
💡 Suggested fix
if (TRUE.equals(recursive)) {
List<DirectoryElementEntity> descendents = repositoryService.findAllDescendants(directoryUuid).stream().toList();
+ if (descendents.isEmpty()) {
+ return List.of();
+ }
// Need to load references for all descendents (no N+1 -> only one query)
List<UUID> ids = descendents.stream().map(DirectoryElementEntity::getId).toList();
- directoryElementRepository.findAllWithReferencesByIdIn(ids);
- return descendents
+ Map<UUID, DirectoryElementEntity> withReferencesById = directoryElementRepository.findAllWithReferencesByIdIn(ids)
+ .stream()
+ .collect(Collectors.toMap(DirectoryElementEntity::getId, e -> e));
+ return descendents
.stream()
+ .map(e -> withReferencesById.getOrDefault(e.getId(), e))
.filter(e -> (types.isEmpty() || types.contains(e.getType())) && permissionService.hasReadPermissions(userId, List.of(e.getId())))
- .map(ElementAttributes::toElementAttributes)
+ .map(ElementAttributes::toElementAttributesWithReferences)
.toList();
}🤖 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 `@src/main/java/org/gridsuite/directory/server/DirectoryService.java` around
lines 271 - 275, The eager-load call
directoryElementRepository.findAllWithReferencesByIdIn(ids) currently has its
result discarded and descendents (from repositoryService.findAllDescendants) is
returned and later transformed with toElementAttributes(), which drops
references; replace that by capturing the result of
findAllWithReferencesByIdIn(ids) (assign it to descendents or a new variable)
and use those returned entities when returning the response, and change the
mapping call from toElementAttributes() to toElementAttributesWithReferences()
so the response includes the loaded references; ensure
repositoryService.findAllDescendants(...) is only used to compute ids, not the
final returned list.
|
|
||
| public static DirectoryElementEntity createRootElement(String elementName, String type, String userId) { | ||
| return new DirectoryElementEntity(UUID.randomUUID(), null, elementName, type, userId, null, Instant.now(), Instant.now(), userId); | ||
| return new DirectoryElementEntity(UUID.randomUUID(), null, elementName, type, userId, null, Instant.now(), Instant.now(), userId, List.of()); |
There was a problem hiding this comment.
Use one timestamp for both creation and modification in test fixtures.
Line 29 and Line 33 call Instant.now() twice, which can produce slightly different values for a freshly created entity and make time-based assertions brittle.
Proposed fix
public static DirectoryElementEntity createRootElement(String elementName, String type, String userId) {
- return new DirectoryElementEntity(UUID.randomUUID(), null, elementName, type, userId, null, Instant.now(), Instant.now(), userId, List.of());
+ Instant now = Instant.now();
+ return new DirectoryElementEntity(UUID.randomUUID(), null, elementName, type, userId, null, now, now, userId, List.of());
}
public static DirectoryElementEntity createElement(UUID parentDirectoryUuid, String elementName, String type, String userId) {
- return new DirectoryElementEntity(UUID.randomUUID(), parentDirectoryUuid, elementName, type, userId, null, Instant.now(), Instant.now(), userId, List.of());
+ Instant now = Instant.now();
+ return new DirectoryElementEntity(UUID.randomUUID(), parentDirectoryUuid, elementName, type, userId, null, now, now, userId, List.of());
}Also applies to: 33-33
🤖 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 `@src/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java`
at line 29, The test fixture creates DirectoryElementEntity with two separate
Instant.now() calls which can differ; capture a single Instant once (e.g.,
Instant now = Instant.now()) in DirectoryTestUtils and use that single timestamp
for both the creation and modification args when constructing
DirectoryElementEntity (apply the same change to both occurrences where two
Instant.now() are passed).
7d1fe6e to
920365a
Compare
SlimaneAmar
force-pushed
the
shared_element
branch
from
920365a to
e488c2f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/gridsuite/directory/server/DirectoryTest.java (1)
240-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedundant duplicate assertion.
Both
checkRootDirectoriesListcalls assert identical content —checkRootDirectoriesListcomparesusingRecursiveComparison().ignoringFieldsOfTypes(Instant.class), so the difference inInstantarguments has no effect. One of these two consecutive assertions can be removed.🧹 Proposed cleanup
checkRootDirectoriesList("userId", List.of(toElementAttributes(uuidNewRootDirectory, "newName", DIRECTORY, "userId", null, creationDateNewDirectory, modificationDateNewDirectory, "userId"))); - - checkRootDirectoriesList("userId", List.of(toElementAttributes(uuidNewRootDirectory, "newName", DIRECTORY, "userId", null, creationDateNewDirectory, creationDateNewDirectory, "userId")));🤖 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 `@src/test/java/org/gridsuite/directory/server/DirectoryTest.java` around lines 240 - 242, The two consecutive assertions calling checkRootDirectoriesList with the same logical content are redundant because checkRootDirectoriesList uses usingRecursiveComparison().ignoringFieldsOfTypes(Instant.class) so the differing Instant values are ignored; remove one of the duplicate calls (either the first or second) that references toElementAttributes(uuidNewRootDirectory, "newName", DIRECTORY, "userId", null, creationDateNewDirectory, modificationDateNewDirectory, "userId")/toElementAttributes(..., creationDateNewDirectory, creationDateNewDirectory, ...) to keep only a single assertion asserting the expected root directory list.
♻️ Duplicate comments (2)
src/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java (1)
28-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a single timestamp for both creation and modification in test fixtures.
Instant.now()is invoked twice in the same expression for bothcreateRootElementandcreateElement, producing slightly different values for a freshly created entity. This can make time-based assertions brittle.🛠️ Proposed fix
public static DirectoryElementEntity createRootElement(String elementName, String type, String userId) { - return new DirectoryElementEntity(UUID.randomUUID(), null, elementName, type, userId, null, Instant.now(), Instant.now(), userId, List.of()); + Instant now = Instant.now(); + return new DirectoryElementEntity(UUID.randomUUID(), null, elementName, type, userId, null, now, now, userId, List.of()); } public static DirectoryElementEntity createElement(UUID parentDirectoryUuid, String elementName, String type, String userId) { - return new DirectoryElementEntity(UUID.randomUUID(), parentDirectoryUuid, elementName, type, userId, null, Instant.now(), Instant.now(), userId, List.of()); + Instant now = Instant.now(); + return new DirectoryElementEntity(UUID.randomUUID(), parentDirectoryUuid, elementName, type, userId, null, now, now, userId, List.of()); }🤖 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 `@src/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java` around lines 28 - 34, The test helpers createRootElement and createElement call Instant.now() twice, producing slightly different createdAt and modifiedAt values; update both methods (DirectoryTestUtils.createRootElement and DirectoryTestUtils.createElement) to capture Instant now = Instant.now() once and reuse that single instant for both the createdAt and modifiedAt parameters so the fixture has identical timestamps for creation and modification.src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml (1)
21-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFK without
onDelete="CASCADE"will break bulk deletes once references exist.
DirectoryElementRepository.deleteAllById(...)(exercised byDirectoryServiceTest.testDeleteMultipleElementsFromOneDirectory) bulk-deletes fromelementdirectly, bypassing JPA cascade on the parent side. Once any element has rows inreference, this constraint will reject the bulk delete. Either add DB-level cascade here or explicitly delete reference rows before the bulk delete path runs.🛡️ Proposed Liquibase fix
- <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true"/> + <addForeignKeyConstraint baseColumnNames="element_id" baseTableName="reference" constraintName="element_id_fk" deferrable="false" initiallyDeferred="false" referencedColumnNames="id" referencedTableName="element" validate="true" onDelete="CASCADE"/>🤖 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 `@src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml` around lines 21 - 22, The foreign key constraint element_id_fk in the Liquibase changeSet (id="1777477329203-3") on table "reference" (baseColumnNames="element_id") lacks onDelete="CASCADE", which causes bulk deletes from the "element" table to fail when related "reference" rows exist; update that changeSet in the changelog to add onDelete="CASCADE" to the <addForeignKeyConstraint> (constraintName="element_id_fk", baseTableName="reference", baseColumnNames="element_id", referencedTableName="element") so the DB will cascade deletes, or alternatively add a new changeset that alters the FK to include onDelete="CASCADE" if the changelog is already applied.
🤖 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 `@src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java`:
- Around line 139-140: The test currently serializes the
ElementAttributes.references collection via its toString() fallback, which can
mis-render non-empty collections; update the assertions in ElementAttributesTest
to serialize references using the existing helper toJsonString("references",
elementAttributes.getReferences()) instead of relying on toString() (apply the
change where references are built around the assertion block, including the
spots mentioned around lines 139 and 147–155), so the test compares proper JSON
output for references.
---
Outside diff comments:
In `@src/test/java/org/gridsuite/directory/server/DirectoryTest.java`:
- Around line 240-242: The two consecutive assertions calling
checkRootDirectoriesList with the same logical content are redundant because
checkRootDirectoriesList uses
usingRecursiveComparison().ignoringFieldsOfTypes(Instant.class) so the differing
Instant values are ignored; remove one of the duplicate calls (either the first
or second) that references toElementAttributes(uuidNewRootDirectory, "newName",
DIRECTORY, "userId", null, creationDateNewDirectory,
modificationDateNewDirectory, "userId")/toElementAttributes(...,
creationDateNewDirectory, creationDateNewDirectory, ...) to keep only a single
assertion asserting the expected root directory list.
---
Duplicate comments:
In `@src/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xml`:
- Around line 21-22: The foreign key constraint element_id_fk in the Liquibase
changeSet (id="1777477329203-3") on table "reference"
(baseColumnNames="element_id") lacks onDelete="CASCADE", which causes bulk
deletes from the "element" table to fail when related "reference" rows exist;
update that changeSet in the changelog to add onDelete="CASCADE" to the
<addForeignKeyConstraint> (constraintName="element_id_fk",
baseTableName="reference", baseColumnNames="element_id",
referencedTableName="element") so the DB will cascade deletes, or alternatively
add a new changeset that alters the FK to include onDelete="CASCADE" if the
changelog is already applied.
In `@src/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java`:
- Around line 28-34: The test helpers createRootElement and createElement call
Instant.now() twice, producing slightly different createdAt and modifiedAt
values; update both methods (DirectoryTestUtils.createRootElement and
DirectoryTestUtils.createElement) to capture Instant now = Instant.now() once
and reuse that single instant for both the createdAt and modifiedAt parameters
so the fixture has identical timestamps for creation and modification.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faf769d3-e6e0-4855-82a8-fa8f09a89e0d
📒 Files selected for processing (11)
src/main/java/org/gridsuite/directory/server/DirectoryService.javasrc/main/java/org/gridsuite/directory/server/dto/ElementAttributes.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementEntity.javasrc/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.javasrc/main/java/org/gridsuite/directory/server/repository/ReferenceEntity.javasrc/main/resources/db/changelog/changesets/changelog_20260429T154156Z.xmlsrc/test/java/org/gridsuite/directory/server/DirectoryServiceTest.javasrc/test/java/org/gridsuite/directory/server/DirectoryTest.javasrc/test/java/org/gridsuite/directory/server/ElementAttributesTest.javasrc/test/java/org/gridsuite/directory/server/PermissionServiceTest.javasrc/test/java/org/gridsuite/directory/server/utils/DirectoryTestUtils.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/gridsuite/directory/server/repository/DirectoryElementRepository.java
Comment on lines
+139
to
140
| toJsonString("references", elementAttributes.getReferences()) | ||
| ) |
There was a problem hiding this comment.
Serialize references collections as JSON, not toString().
After Line 139, non-empty references can be mis-rendered by the current fallback in Line 155, causing brittle/incorrect test assertions.
💡 Suggested fix
private String toJsonString(String key, Object value) {
if (value == null) {
return (String) value;
}
if (value instanceof Instant) {
DateTimeFormatter formatter = DateTimeFormatter.ISO_INSTANT;
return String.format("\"%s\":\"%s\"", key, formatter.format((Instant) value));
}
+ if (value instanceof Collection<?>) {
+ try {
+ return String.format("\"%s\":%s", key, mapper.writeValueAsString(value));
+ } catch (Exception e) {
+ throw new IllegalStateException("Unable to serialize collection for key: " + key, e);
+ }
+ }
return String.format(value instanceof String || value instanceof UUID ? "\"%s\":\"%s\"" : "\"%s\":%s", key, value);
}Also applies to: 147-155
🤖 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 `@src/test/java/org/gridsuite/directory/server/ElementAttributesTest.java`
around lines 139 - 140, The test currently serializes the
ElementAttributes.references collection via its toString() fallback, which can
mis-render non-empty collections; update the assertions in ElementAttributesTest
to serialize references using the existing helper toJsonString("references",
elementAttributes.getReferences()) instead of relying on toString() (apply the
change where references are built around the assertion block, including the
spots mentioned around lines 139 and 147–155), so the test compares proper JSON
output for references.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary