Remove directory server dependency#193
Conversation
📝 WalkthroughWalkthroughThe PR removes the DirectoryService and its ElementAttributes DTO, simplifies the /filters/infos endpoint to drop the ChangesFilter service + API refactor
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 2
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/filter/server/FilterService.java (1)
69-72: Use aSetfor missing-UUID lookup to avoid quadratic scans.
foundUuids.contains(...)on aListmakes the missing-ID pass O(n*m). Converting to aSetkeeps this efficient for larger batches.♻️ Proposed refactor
- List<UUID> foundUuids = filterAttributes.stream().map(FilterAttributes::getId).toList(); - List<UUID> notFoundUuids = filterUuids.stream().filter(filterUuid -> !foundUuids.contains(filterUuid)).toList(); + Set<UUID> foundUuids = filterAttributes.stream() + .map(FilterAttributes::getId) + .collect(Collectors.toSet()); + List<UUID> notFoundUuids = filterUuids.stream() + .filter(filterUuid -> !foundUuids.contains(filterUuid)) + .toList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/filter/server/FilterService.java` around lines 69 - 72, The current missing-UUID detection in FilterService uses a List (foundUuids) and calls foundUuids.contains(...) inside the filterUuids stream, causing O(n*m) behavior; change the intermediate collection to a Set for O(1) lookups: build a Set<UUID> (e.g., foundUuidSet from filterAttributes.stream().map(FilterAttributes::getId)) and then compute notFoundUuids by filtering filterUuids with !foundUuidSet.contains(uuid) (keep the variable names filterAttributes, filterUuids, notFoundUuids, and use FilterAttributes::getId to locate the mapping).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/org/gridsuite/filter/server/FilterServiceTest.java`:
- Line 47: Rename the test method getCyclicFilterNamesUsesResolvedIdsAndIds to
reflect ID-based behavior (for example getCyclicFilterIdsUsesResolvedIdsAndIds
or getCyclicFilterChainIdsUsesResolvedIdsAndIds), update the method declaration
and any references/annotations/usages in FilterServiceTest so the name matches
that assertions validate UUID chain formatting, and run the test suite to ensure
there are no remaining references to the old method name.
- Line 43: The test FilterServiceTest constructs filterService with potentially
null collaborators because the `@Mock` fields are never initialized; add explicit
mock initialization by calling MockitoAnnotations.openMocks(this) in a
`@BeforeEach` method (or replace field `@Mock` annotations with Spring `@MockBean`) so
repositoryService, networkStoreService, and notificationService are non-null
when used in the FilterService constructor; also consider renaming the test
method invokeGetCyclicFilterNames to invokeGetCyclicFilterIds to match its
ID-based behavior.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/server/FilterService.java`:
- Around line 69-72: The current missing-UUID detection in FilterService uses a
List (foundUuids) and calls foundUuids.contains(...) inside the filterUuids
stream, causing O(n*m) behavior; change the intermediate collection to a Set for
O(1) lookups: build a Set<UUID> (e.g., foundUuidSet from
filterAttributes.stream().map(FilterAttributes::getId)) and then compute
notFoundUuids by filtering filterUuids with !foundUuidSet.contains(uuid) (keep
the variable names filterAttributes, filterUuids, notFoundUuids, and use
FilterAttributes::getId to locate the mapping).
🪄 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: 0fec1c05-1aba-456e-b924-a0b4185ff91c
📒 Files selected for processing (7)
src/main/java/org/gridsuite/filter/server/FilterController.javasrc/main/java/org/gridsuite/filter/server/FilterService.javasrc/main/java/org/gridsuite/filter/server/dto/ElementAttributes.javasrc/main/java/org/gridsuite/filter/server/service/DirectoryService.javasrc/main/resources/application-local.yamlsrc/test/java/org/gridsuite/filter/server/FilterEntityControllerTest.javasrc/test/java/org/gridsuite/filter/server/FilterServiceTest.java
💤 Files with no reviewable changes (4)
- src/main/java/org/gridsuite/filter/server/dto/ElementAttributes.java
- src/main/resources/application-local.yaml
- src/test/java/org/gridsuite/filter/server/FilterEntityControllerTest.java
- src/main/java/org/gridsuite/filter/server/service/DirectoryService.java
| @BeforeEach | ||
| void setUp() { | ||
| filterService = new FilterService(repositoryService, networkStoreService, notificationService, directoryService); | ||
| filterService = new FilterService(repositoryService, networkStoreService, notificationService); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="$(fd -a 'FilterServiceTest.java' | head -n1)"
echo "Inspecting: $FILE"
# Show test wiring annotations and mock initialization patterns
rg -n '@SpringBootTest|@Mock\b|@MockBean\b|@ExtendWith|MockitoAnnotations\.openMocks|MockitoExtension' "$FILE"Repository: gridsuite/filter-server
Length of output: 220
🏁 Script executed:
cat -n /home/jailuser/git/src/test/java/org/gridsuite/filter/server/FilterServiceTest.java | sed -n '25,55p'Repository: gridsuite/filter-server
Length of output: 1169
🏁 Script executed:
rg -n 'MockitoAnnotations|@ExtendWith|@BeforeAll|@BeforeClass' /home/jailuser/git/src/test/java/org/gridsuite/filter/server/FilterServiceTest.javaRepository: gridsuite/filter-server
Length of output: 49
🏁 Script executed:
cat -n /home/jailuser/git/src/test/java/org/gridsuite/filter/server/FilterServiceTest.java | sed -n '47,75p'Repository: gridsuite/filter-server
Length of output: 1522
Mock collaborators are uninitialized in this test setup.
The @Mock fields lack proper initialization under @SpringBootTest: there is no @ExtendWith(MockitoExtension.class), no MockitoAnnotations.openMocks() call, and no @MockBean annotation. Line 43 passes potentially null parameters to the FilterService constructor, creating brittle test wiring. The test passes only because the current assertions never invoke methods on these collaborators. Initialize mocks explicitly via MockitoAnnotations.openMocks() in a @BeforeEach method or use Spring @MockBean annotations to ensure stable test behavior.
As a secondary note, the test method name at line 47 references "Names" but the test behavior is ID-based (invokeGetCyclicFilterIds); consider renaming for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/gridsuite/filter/server/FilterServiceTest.java` at line 43,
The test FilterServiceTest constructs filterService with potentially null
collaborators because the `@Mock` fields are never initialized; add explicit mock
initialization by calling MockitoAnnotations.openMocks(this) in a `@BeforeEach`
method (or replace field `@Mock` annotations with Spring `@MockBean`) so
repositoryService, networkStoreService, and notificationService are non-null
when used in the FilterService constructor; also consider renaming the test
method invokeGetCyclicFilterNames to invokeGetCyclicFilterIds to match its
ID-based behavior.
Co-authored-by: carojeandat <112859659+carojeandat@users.noreply.github.com>
|
| public List<FilterAttributes> getFiltersAttributes(List<UUID> filterUuids, String userId) { | ||
| public List<FilterAttributes> getFiltersAttributes(List<UUID> filterUuids) { | ||
| List<FilterAttributes> filterAttributes = this.repositoriesService.getFiltersAttributes(filterUuids).collect(Collectors.toList()); | ||
| // call directory server to add name information |
There was a problem hiding this comment.
Consider to remove name in the library filter:
https://github.com/gridsuite/filter/blob/main/src/main/java/org/gridsuite/filter/identifierlistfilter/FilterAttributes.java#L34
There was a problem hiding this comment.
may be done in a next PR to avoid complexity in this ticket
There was a problem hiding this comment.
Co-authored-by: Thang PHAM <117309322+thangqp@users.noreply.github.com>
|
There was a problem hiding this comment.
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/filter/server/FilterServiceTest.java (1)
72-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsafe
Throwable → Exceptioncast will maskErrorfailures with aClassCastException.If the reflected invocation ever throws an
Errorsubtype (e.g.,AssertionError,OutOfMemoryError),targetException.getCause()returns aThrowable, and(Exception) causewill itself throw aClassCastException, hiding the original failure entirely.🐛 Proposed fix
} catch (InvocationTargetException targetException) { Throwable cause = targetException.getCause(); - throw (Exception) cause; + if (cause instanceof Exception e) { + throw e; + } + throw new RuntimeException("Unexpected Error from reflected invocation", cause); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/filter/server/FilterServiceTest.java` around lines 72 - 75, In the catch for InvocationTargetException in FilterServiceTest, avoid casting the Throwable from targetException.getCause() directly to Exception; instead check the cause: if it's an Exception rethrow it, if it's an Error rethrow it as an Error, otherwise wrap the Throwable in a new Exception and throw that (use the symbols InvocationTargetException, targetException.getCause(), and the test method in FilterServiceTest to locate the catch block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/test/java/org/gridsuite/filter/server/FilterServiceTest.java`:
- Around line 72-75: In the catch for InvocationTargetException in
FilterServiceTest, avoid casting the Throwable from targetException.getCause()
directly to Exception; instead check the cause: if it's an Exception rethrow it,
if it's an Error rethrow it as an Error, otherwise wrap the Throwable in a new
Exception and throw that (use the symbols InvocationTargetException,
targetException.getCause(), and the test method in FilterServiceTest to locate
the catch block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec7a3934-dccf-4fd8-8991-c80d8cea537e
📒 Files selected for processing (1)
src/test/java/org/gridsuite/filter/server/FilterServiceTest.java
3 participants
PR Summary