Development: Enforce Usage of Optional<T> Over @RequestParam(required = false) in Method Parameters

[pvlov]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• This is a small issue that I tested locally and was confirmed by another developer on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

Motivation and Context

There are some instances where a Controller/Endpoint uses the @RequestParam annotation with required set to false. This however leads to the issue of potential NullPointerExceptions. Spring supports the use of Optional<T> for this use case, leading to a Optional.empty() if the value is not set. This would allow for cleaner functional-style code.

Description

This PR introduces a new ArchUnit test to enforce a coding standard regarding the usage of @RequestParam annotations in method parameters. The test ensures that no method within the specified packages uses @RequestParam with required = false. Instead, it encourages the use of Optional to handle optional parameters.

Steps for Testing

Run the ArchTests, probably using ./gradlew test.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.

Review Progress

Performance Review

• I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
• I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Exam Mode Test

• Test 1
• Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

• New Features

  • Enhanced API design by favoring Optional<T> for method parameters, improving clarity and safety.
  • Introduced default values for certain parameters to ensure consistent behavior and reduce ambiguity.

• Bug Fixes

  • Improved handling of parameters to reduce the risk of null-related issues.

• Enhancements

  • Updated method signatures to replace nullable types with Optional<T>, promoting better null safety and code readability.

[coderabbitai]

Walkthrough

The recent updates across various classes enhance the handling of optional parameters by transitioning from nullable types to Optional<T> types. This change improves code clarity and safety, explicitly indicating the potential absence of a value. The modifications support better API design practices, reducing the risk of NullPointerExceptions and promoting a more functional programming style.

Changes

Files	Change Summary
src/main/java/de/tum/in/www1/artemis/web/rest/ComplaintResource.java, src/main/java/de/tum/in/www1/artemis/web/rest/repository/RepositoryProgrammingExerciseParticipationResource.java	Updated methods to use Optional<Long> for parameters instead of nullable types, improving clarity and null safety.
src/main/java/de/tum/in/www1/artemis/web/rest/ModelingExerciseResource.java, src/main/java/de/tum/in/www1/artemis/web/rest/TextExerciseResource.java, src/main/java/de/tum/in/www1/artemis/web/rest/QuizExerciseResource.java	Refactored methods to manage notificationText as Optional<String> and boolean parameters using primitives with defaults, simplifying logic and ensuring initialization.
src/main/java/de/tum/in/www1/artemis/web/rest/metis/conversation/ConversationResource.java	Changed the filter parameter to Optional<ConversationMemberSearchFilters>, clarifying its optional nature and improving method usability.
src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseService.java, src/main/java/de/tum/in/www1/artemis/web/rest/programming/ProgrammingExerciseResource.java	Transitioned notificationText parameters from nullable to Optional<String>, enhancing type safety and reducing null handling complexity.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API
    participant Service

    Client->>API: Call endpoint with optional parameters
    API->>Service: Process request with Optional parameters
    Service-->>API: Return results or handle absence
    API-->>Client: Send response back

Loading

Possibly related issues

• Programing exercise: Student does not see the results from the latest submission in the overview #9086: The changes in handling optional parameters could contribute to resolving the issue where the latest submission results are not displayed correctly, potentially linked to non-unique results in the database.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[pvlov]

This was very briefly discussed with @Strohgelaender. As of now, this PR does not actually fix any violation of this rule. Keeping the single responsibility principle in mind, the PR actually fixing the violations should be separate, as the refactoring might introduce many code changes to incorporate the change.

[coderabbitai]

Actionable comments posted: 1

[coderabbitai]

Actionable comments posted: 1

[Strohgelaender]

Thank you for the contribution and welcome to Artemis 👋

Please also fix the violations of the new test in this PR. We don't want to merge PRs that contain test failures into develop.

Also @RequestParam(required=false) Optional<T> would currently lead to a violation, but I don't think this should be ok.

[krusche]

@pvlov after discussing this issue again with @dfuchss we came to the following conclusion:

1. We want to use Optional for method parameters in REST Controllers when it makes sense (in particular for non required request params)
2. We do not want to use it for method parameters anywhere else, in particular not for Services

https://dzone.com/articles/java-8-optional-how-use-it clearly states:
Optional is not meant to be used in these contexts, as it won't buy us anything:

• in the domain model layer (not serializable)
• in DTOs (same reason)
• in input parameters of methods
• in constructor parameters

I generally see two exceptions and kindly ask you to update the server coding guidelines as well including examples:

1. Optional services (e.g. due to specific profiles being or not being active, e.g. Optional<VersionControlService> versionControlService in ProgrammingSubmissionService)
2. Input parameters of methods for RestControllers, e.g. @RequestParam(required = false) Optional<Long> lectureId in LectureResource

All other uses of Optional in constructor parameters and in input parameters of methods should actually be removed in the future

[dfuchss]

I'd suggest that this PR also introduces an architecture test that ensures the correct use of optional as parameters

[github-actions]

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

[pvlov]

@pvlov after discussing this issue again with @dfuchss we came to the following conclusion:

1. We want to use Optional for method parameters in REST Controllers when it makes sense (in particular for non required request params)
2. We do not want to use it for method parameters anywhere else, in particular not for Services

https://dzone.com/articles/java-8-optional-how-use-it clearly states: Optional is not meant to be used in these contexts, as it won't buy us anything:

• in the domain model layer (not serializable)
• in DTOs (same reason)
• in input parameters of methods
• in constructor parameters

I generally see two exceptions and kindly ask you to update the server coding guidelines as well including examples:

1. Optional services (e.g. due to specific profiles being or not being active, e.g. Optional<VersionControlService> versionControlService in ProgrammingSubmissionService)
2. Input parameters of methods for RestControllers, e.g. @RequestParam(required = false) Optional<Long> lectureId in LectureResource

All other uses of Optional in constructor parameters and in input parameters of methods should actually be removed in the future

While I still disagree with this stance, especially since you lose the ability to assert when a given parameter has been checked for null(at the type level) and your decision about not allowing Optionals in non-REST contexts will lead to opt.orElse(null), I'll accept it in the interest of finally getting this PR merged.

Regarding the documentation request, I assume you want me to change the documentation in confluence? Is there a certain procedure to follow when updating documentation I should be aware of?

I was and still am in Japan until the end of the week and will probably address the change requests in the upcoming week.

[pvlov]

I'd suggest that this PR also introduces an architecture test that ensures the correct use of optional as parameters

Should the PR really get any bigger at this point? I think this could and should be handled as a separate PR. It would also be a good first issue for any other contributor. This should also line up with what @krusche meant with "All other uses of Optional in constructor parameters and in input parameters of methods should actually be removed in the future".

[pvlov]

@krusche @dfuchss Could you please go over the new addition to the server guidelines in this PR and check whether you agree with the wording?

Furthermore I have an additional question: How should we handle the case where using Optional is not ergonomic? i.e. in the case of notificationText, where the null case is handled pretty deep down the method chain. I have two suggestions:

1. Just stay with Optional and unwrap the Optional with opt.orElse(null).
2. Add the checker annotation @Nullable to get the ball rolling in regards to the use of the checker framework and add it as an Exception to the Exception of the rule. This would keep the old logic with required = false, but would add the possibility of using the checker framework on it and act as a makeshift documentation.

[dfuchss]

@krusche @dfuchss Could you please go over the new addition to the server guidelines in this PR and check whether you agree with the wording?

Furthermore I have an additional question: How should we handle the case where using Optional is not ergonomic? i.e. in the case of notificationText, where the null case is handled pretty deep down the method chain. I have two suggestions:

1. Just stay with Optional and unwrap the Optional with opt.orElse(null).
2. Add the checker annotation @Nullable to get the ball rolling in regards to the use of the checker framework and add it as an Exception to the Exception of the rule. This would keep the old logic with required = false, but would add the possibility of using the checker framework on it and act as a makeshift documentation.

For me, the stated guidelines look good.

[dfuchss]

I'd suggest that this PR also introduces an architecture test that ensures the correct use of optional as parameters

Should the PR really get any bigger at this point? I think this could and should be handled as a separate PR. It would also be a good first issue for any other contributor. This should also line up with what @krusche meant with "All other uses of Optional in constructor parameters and in input parameters of methods should actually be removed in the future".

I mean the question might be "how many are there". If it's kind of e.g., 10 I'd suggest to do that. otherwise, I'd suggest to add a PR later. In any case I'd add an architecture test to check for wrong occurrences.