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: Improve XML parsing #8462

Merged
merged 62 commits into from
May 21, 2024
Merged

Development: Improve XML parsing #8462

merged 62 commits into from
May 21, 2024

Conversation

krusche
Copy link
Member

@krusche krusche commented Apr 23, 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 added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • 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

The processing of result artifacts (test results, sca reports) currently uses some custom XML parsing logic. This can get quite complex and error-prone. It also makes it hard to add new supported formats or to adjust to external changes.

Description

This PR removes the custom parsing logic and instead uses the Jackson object mapper with records.

Steps for Testing

Participate in a Programming exercise as a student. Make sure that test results and SCA feedback are visible and correct.

Exam Mode Testing

same for exam exercises

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

Exam Mode Test

  • Test 1
  • Test 2

Test Coverage

unchanged

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) labels Apr 23, 2024
@Strohgelaender Strohgelaender marked this pull request as ready for review April 23, 2024 20:16
Copy link
Contributor

@laadvo laadvo 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 ts2, test result and SCA when participating in a programming exercise work as expected.

Copy link
Contributor

@marlon-luca-bu marlon-luca-bu 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 TS2 and works as expected 👍

Copy link
Contributor

@Jan-Thurner Jan-Thurner left a comment

Choose a reason for hiding this comment

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

Tested the exam mode on TS1. Works as expected.

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.

Tested again on TS1 with a programming exercise with SCA ✅

@krusche krusche merged commit ee5274e into develop May 21, 2024
74 of 78 checks passed
@krusche krusche deleted the chore/xmlmapper branch May 21, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants