-
Notifications
You must be signed in to change notification settings - Fork 7
Replace Orika mapping with MapStruct #720
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #720 +/- ##
==========================================
- Coverage 37.22% 36.46% -0.77%
==========================================
Files 536 530 -6
Lines 23706 23560 -146
Branches 2857 2823 -34
==========================================
- Hits 8825 8590 -235
- Misses 14003 14104 +101
+ Partials 878 866 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This allows ContentBase types to be returned from a normal subclass mapping method without causing abstract interface errors
Since this actually handles quiz mappings too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Very bulky, but needed to stop warnings
This removes the old Orika converters & automappers and replaces them with MapStruct mappings.
Most of the mappings just define a source & target type and let the MapStruct auto-implementations handle the rest. Where the target class is unambiguous (e.g. DTO <-> DO mappings), using these mappings is then as simple as calling
map(source)
(orcopy(source)
if the source & target classes are the same); the target class no longer needs to be specified. Where there is ambiguity in which class to map to (e.g.map(ContentDTO)
), the defaultmap
method does still need to have a target class passed in to know which mapping to delegate to. In the case of mapping an object to a String (mapContentSummaryDTOtoString
), the implementation is hardcoded rather than autogenerated; see mapstruct/mapstruct#584.The mappers are split roughly according to the kind of objects they map, but the
MainMapper
interface is used to provide a mapper object in most cases. For consistency, one of the new mappers is calledContentMapper
, hence the existingContentMapper
has been renamed (toContentSubclassMapper
).Mappings are provided for all of the conversions that we currently need. These are not comprehensive, and there are likely other mappings that will be needed at some point that should be added now.
As discussed, there are no new unit tests introduced by this PR. The existing integration tests should cover the usage of most of the new mappings but there may be cases that are missed.