Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Development: Add dynamic fetches to simplify complexity in repositories #8607

Merged
merged 25 commits into from
Jun 20, 2024

Conversation

krusche
Copy link
Member

@krusche krusche commented May 16, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

  • High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).
  • I tested all changes and their related features with all corresponding user types on a test server configured with Gitlab and Jenkins.

Motivation and Context

Currently we have some repositories that use a lot of different methods to fetch an entity by an ID while eagerly fetching different relationships.
This leads to needlessly large repositories.

Description

To address this issue we add a new mechanism to eagerly fetch relationships using specifications.
You can read the new documentation section for details, but effectively it does this:

  • You specify which relationships to fetch using an enum
  • The repository method constructs a specification from it and fetches the based on this

Steps for Testing

Prerequisites:

  • 1 Instructor
  1. Log in to Artemis
  2. Go to the instructor detail page of a programming exercise
  3. You can see all information as you would on develop

Testserver States

Note

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






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

Test Coverage

Summary by CodeRabbit

  • New Features

    • Introduced dynamic fetching methods for loading entity relationships on demand, enhancing the flexibility and efficiency of data retrieval.
    • Added methods for finding entities by ID with specific fetch options, improving data querying capabilities.
    • Enhanced the loading functionality of programming exercises with additional criteria like submission results and grading criteria.
  • Documentation

    • Updated developer guidelines to include new methods for dynamic entity fetching.

@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label May 16, 2024
@krusche krusche changed the title Development: Add dynamic fetches Development: Add dynamic fetches to simplify complexity in repositories May 16, 2024
@MichaelOwenDyer
Copy link
Contributor

Looks great so far, I fully support this idea. I think it would be even more ergonomic for the developer if the fetch options could be provided as varargs as well as/instead of inside a collection. I think most of the time these fetch options are going to be specified inline in the method arguments, and requiring a collection would mean that you have to wrap them in List.of or similar every time, which in my opinion is a nuisance.

@krusche
Copy link
Member Author

krusche commented May 16, 2024

Looks great so far, I fully support this idea. I think it would be even more ergonomic for the developer if the fetch options could be provided as varargs as well as/instead of inside a collection. I think most of the time these fetch options are going to be specified inline in the method arguments, and requiring a collection would mean that you have to wrap them in List.of or similar every time, which in my opinion is a nuisance.

Thanks for the positive feedback 👍 I actually had a varargs version as well :-) but it was unused, so I decided to delete it again before pushing the code. We can definitely add one and support the "empty" fetch as well.

I re-added the varargs version in my latest commit

coderabbitai[bot]
coderabbitai bot previously approved these changes May 16, 2024
Copy link

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.

@github-actions github-actions bot added the stale label May 25, 2024
@krusche krusche removed the stale label Jun 2, 2024
Copy link

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.

@github-actions github-actions bot added the stale label Jun 10, 2024
@Hialus Hialus marked this pull request as ready for review June 15, 2024 16:11
Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed the usage of generics for the IDs:

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good 👍

Copy link
Contributor

@coolchock coolchock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested on ts4, the programming exercise details page appears as is does on develop

Copy link
Contributor

@JohannesWt JohannesWt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS4 Instructor Prog. Exercise View has no issues

Copy link
Contributor

@MichaelOwenDyer MichaelOwenDyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

Copy link
Contributor

@sarpsahinalp sarpsahinalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on ts4. Detail page of programming exercise works as expected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation ready for review server Pull requests that update Java code. (Added Automatically!)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants