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: Reduce fetched data for get competency endpoint #7460

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

MaximilianAnzinger
Copy link
Collaborator

@MaximilianAnzinger MaximilianAnzinger commented Oct 29, 2023

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.
  • I chose a title conforming to the naming conventions for pull requests.

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines.
  • I documented the Java code using JavaDoc style.

Motivation and Context

Currently the endpoint getCompetency(long) fetches data for all users. This PR reduces the fetched data to only contain information related to the user sending the request, i.e. it excludes any other users' progress enteties.

Description

Optimized query to only fetch user related data.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student
  • 1 Competency with Exercises and Lecture Units attached
  1. Log in to Artemis as an Isntructor
  2. Navigate to Course Administration > Competency Management, view and edit the competencies, make sure nothing breaks
  3. Navigate to the competency tab
  4. Make sure you can see all competencies correctly
  5. Click on one of the competencies and make sure the competency card is correctly displayed
  6. Check that the request to retrieve the competency information does NOT contain information, e.g. progress data, of other users.

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 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

@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Oct 29, 2023
""")
Optional<Competency> findByIdWithExercisesAndLectureUnits(@Param("competencyId") Long competencyId);
@EntityGraph(type = LOAD, attributePaths = { "userProgress", "lectureUnits.completedUsers" })
Copy link
Member

Choose a reason for hiding this comment

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

is this EntityGraph really needed? I think we want to avoid loading all lectureUnits.completedUsers
Can you not simply use left join fetch as above but add a WHERE clause?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fetching all data is prevented by joining on userId. Adding this to the WHERE clause would incorrectly return the competency only when a progress entity for the user exists. If not, the query would not find it and return empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use LEFT JOIN FETCH ... ON in the query instead of the entity graph?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what I tried at first. It's not supported.

Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment above? I think the statement is not fully clear otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

--> follow-up

Copy link
Contributor

@lennart-keller lennart-keller 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 ts1. Everything seems to still work fine. Response now only contains one userProgress element.

Copy link
Contributor

@Strohgelaender Strohgelaender 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

@tobias-lippert tobias-lippert left a comment

Choose a reason for hiding this comment

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

Changed Query LGTM

Copy link
Member

@bassner bassner 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 TS1, the request only returns my own progress. LGTM 👍

Copy link
Contributor

@rstief rstief 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 ts1, everything works & request only returns my data.

Copy link

@vinceclifford vinceclifford left a comment

Choose a reason for hiding this comment

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

Manually tested on a legacy server. Everything works as despribed

@krusche krusche added this to the 6.6.3 milestone Oct 29, 2023
Copy link

@JanaNF JanaNF left a comment

Choose a reason for hiding this comment

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

Manual tested on legacy server ts2. Problems with the student view.

As a student (tried with different test accounts) I saw the "Edit" button and even came to the "Edit a competency" page. There I got the message "You are not authorized" and I couldn't change anything but I don't think a student user should see it. I tried this in different courses and it is always the same. The student user can even see the "Competency Management" page

Please let me know if this has anything to do with your PR.

@krusche krusche merged commit 1119515 into develop Oct 29, 2023
64 of 66 checks passed
@krusche krusche deleted the bugfix/get-competency branch October 29, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix priority:high ready to merge server Pull requests that update Java code. (Added Automatically!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants