einhaltung der abhängigkeiten vorfälle service #1042
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the security-related components of the project by reorganizing the package structure and incorporating a user authority service. The Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wls-vorfaelleundvorkommnisse-service/src/test/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/archunit/ArchUnitTest.java (1)
73-100: Inconsistent access modifier for rule provider methodI noticed that
allClassesWithoutTestsRulesToVerify()is declared aspublicwhileallClassesRulesToVerify()on line 65 isprivate. Consider using consistent access modifiers unless there's a specific reason for the difference.- public static Stream<Arguments> allClassesWithoutTestsRulesToVerify() { + private static Stream<Arguments> allClassesWithoutTestsRulesToVerify() {wls-vorfaelleundvorkommnisse-service/src/test/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/service/security/UserInfoAuthoritiesServiceTest.java (1)
110-111: Minor Typo in Variable Name:
The variable name "expctedAuthorities" appears to be misspelled. Renaming it to "expectedAuthorities" would improve readability and consistency with naming conventions.wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/configuration/SecurityConfiguration.java (1)
57-60: Consider Leveraging Dependency Injection for UserInfoAuthoritiesServiceCurrently, the code instantiates
UserInfoAuthoritiesServicedirectly within theJwtUserInfoAuthenticationConverterconstructor. To fully benefit from Spring's dependency injection (including lifecycle management, proxy support, and easier testing), consider definingUserInfoAuthoritiesServiceas a Spring bean and then injecting it. For example, you might add a bean definition as follows:+ @Bean + public UserInfoAuthoritiesService userInfoAuthoritiesService() { + return new UserInfoAuthoritiesService(userInfoUri, restTemplateBuilder); + }And update the converter instantiation:
- new JwtUserInfoAuthenticationConverter(new UserInfoAuthoritiesService(userInfoUri, restTemplateBuilder)) + new JwtUserInfoAuthenticationConverter(userInfoAuthoritiesService())This refactor will help maintain cleaner dependency management and adhere even more closely to the project’s architectural rules.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/configuration/JwtUserInfoAuthenticationConverter.java(1 hunks)wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/configuration/SecurityConfiguration.java(1 hunks)wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/service/security/UserInfoAuthoritiesService.java(1 hunks)wls-vorfaelleundvorkommnisse-service/src/test/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/archunit/ArchUnitTest.java(4 hunks)wls-vorfaelleundvorkommnisse-service/src/test/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/service/security/UserInfoAuthoritiesServiceTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/configuration/JwtUserInfoAuthenticationConverter.java (2)
Learnt from: MrSebastian
PR: it-at-m/Wahllokalsystem#603
File: wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/JwtUserInfoAuthenticationConverter.java:27-29
Timestamp: 2025-03-14T05:04:11.799Z
Learning: In the `JwtUserInfoAuthenticationConverter` class (`wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/JwtUserInfoAuthenticationConverter.java`), null checks for the `Jwt` parameter in the `convert(Jwt source)` method are unnecessary because the `Converter` interface specifies that the `source` parameter is never null.
Learnt from: MrSebastian
PR: it-at-m/Wahllokalsystem#603
File: wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/JwtUserInfoAuthenticationConverter.java:27-29
Timestamp: 2025-03-14T05:03:56.221Z
Learning: In the `JwtUserInfoAuthenticationConverter` class, the method `UserService.getUserDetails(String subject)` throws an exception if the user is not found and does not return null. Therefore, checking for a null `userDetails` is unnecessary.
🔇 Additional comments (9)
wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/configuration/JwtUserInfoAuthenticationConverter.java (2)
7-7: Updated Dependency Import CorrectlyThe import now fetches
UserInfoAuthoritiesServicefrom theservice.securitypackage, which aligns with the new dependency management rule and improved architectural grouping. Please ensure that the package path is correct and that all affected modules are updated accordingly.
32-35: Effective Use of the New Authority ServiceThe
convertmethod is cleanly updated to callthis.userInfoService.loadAuthorities(source)when constructing theJwtAuthenticationToken. This leverages the new service as intended and correctly omits any unnecessary null checks, in accordance with the guidance from previous learnings.wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/service/security/UserInfoAuthoritiesService.java (1)
5-5: Updated Package Declaration is Correctly ImplementedThe package declaration has been updated to reflect the new security service context:
package de.muenchen.oss.wahllokalsystem.vorfaelleundvorkommnisseservice.service.security;This change aligns with the project naming conventions and supports the architectural reorganization described in the PR objectives. Please ensure that all files importing this class (e.g.,
JwtUserInfoAuthenticationConverter.java,SecurityConfiguration.java, and the corresponding test file) have been updated to use the new package.wls-vorfaelleundvorkommnisse-service/src/test/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/archunit/ArchUnitTest.java (4)
21-22: Nice addition of the filter for non-test classesThe addition of
allClassesWithoutTestsandignoreGeneratedCodeprovides a clean way to focus architectural rule validation specifically on production code while excluding test classes and generated code.
33-36: Good implementation of the ClassFileImporterThe configuration for the non-test classes importer follows the established pattern in the file and correctly applies both import options to filter out test classes and generated code.
51-55: Well-structured test method for non-test classesThe parameterized test method is appropriately named and implemented consistently with the existing test methods in the class.
73-100: Comprehensive set of architectural rulesThe implementation includes a well-organized set of rules categorized by architectural layers (REST, service, domain). This comprehensive coverage will help maintain clear architectural boundaries in the codebase, which aligns with the PR objective of adding dependency management rules.
wls-vorfaelleundvorkommnisse-service/src/test/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/service/security/UserInfoAuthoritiesServiceTest.java (1)
1-1: Update of Package Declaration:
The package declaration has been updated to reflect the new reorganization — moving the security-related service from the configuration package to the service.security package. This change aligns well with the PR objectives on dependency management and architectural clarity.wls-vorfaelleundvorkommnisse-service/src/main/java/de/muenchen/oss/wahllokalsystem/vorfaelleundvorkommnisseservice/configuration/SecurityConfiguration.java (1)
7-7: New Service Import Reflects Package ReorganizationThe added import for
UserInfoAuthoritiesServiceconfirms the migration to the dedicated service package, aligning well with the dependency management and architectural guidelines specified in the PR objectives.
vjohnslhm
changed the title
…ltung-der-abhängigkeiten
There was a problem hiding this comment.
Warum wurde der hierher verschoben? Ist ja eigentlich kein "normaler" Service mit @Service Annotation und wird auch nicht vom Controller verwendet. Vielleicht würde es Sinn machen diese Klasse in "UserInfoAuthoritiesProvider" umzubenennen - das Problem haben wir nämlich bei anderen Services auch.
There was a problem hiding this comment.
Hab in dem MR das gleiche problem gehabt. Hatte damals mit @MrSebastian besprochen, dass wir es in den Service-Ordner schieben. Die idee mit Provider statt Service find ich auch nicht schelcht. Für mich passt beides, hauptsache einheitlich. @MrSebastian, da wir es besprochen hatten, was ist deine Meinung?
Beschreibung:
Definition of Done (DoD):
Backend
Referenzen1:
Closes #1010
Summary by CodeRabbit
Refactor
UserInfoAuthoritiesServiceand its test to reflect a more logical structure within the security context.Tests
These behind-the-scenes improvements enhance system maintainability and reliability without altering any user-facing functionality.
Footnotes
Nicht zutreffende Referenzen vor dem Speichern entfernen ↩