Skip to content

Supported nested conversions for AggregateReference. #2062

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Supported nested conversions for AggregateReference. #2062

wants to merge 5 commits into from

Conversation

schauder
Copy link
Contributor

Restructured reading conversion process into:

  • converting technology base types (JDBC Arrays).
  • standard and custom conversions.
  • module specific conversions (AggregateReference).

Closes #1828
@schauder schauder requested a review from mp911de May 23, 2025 10:10
@mp911de mp911de self-assigned this May 26, 2025
mp911de
mp911de reviewed May 26, 2025
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

I left a few comments. I assumed this PR would had addressed the issue of late converter registration so that we can remove AggregateReferenceConverters by considering AggregateReference in writeValue (and readValue). We should revisit those converters for having a cleaner approach and to not distribute conversion of AggregateReference to components outside of MappingRelationalConverter/MappingJdbcConverter.
@mp911de mp911de added the type: bug A general bug label May 26, 2025
@schauder schauder assigned schauder and unassigned mp911de May 27, 2025
schauder added a commit that referenced this pull request May 27, 2025
@schauder
Support AggregateReferences with custom id type.
93d2aaf 
Restructured reading conversion process into: - converting technology base types (JDBC Arrays). - standard and custom conversions. - module specific conversions (AggregateReference). Closes #1828 Original pull request #2062
schauder added a commit that referenced this pull request May 27, 2025
@schauder
Process review comments.
fe66a43 
- Wrapped SqlException and rethrow it. - remove AggregateReferenceConverters completely. It was no longer used. Closes #1828 Original pull request #2062
@schauder schauder force-pushed the issue/1828-aggregateref-new branch from 7ef8ac0 to fe66a43 Compare May 27, 2025 08:21
schauder added a commit that referenced this pull request May 27, 2025
@schauder
Simplified structure of MappingRelationalConverter.readValue().
483808f 
See #1828 Original pull request #2062
@schauder
Copy link
Contributor Author

I left a few comments. I assumed this PR would had addressed the issue of late converter registration so that we can remove AggregateReferenceConverters by considering AggregateReference in writeValue (and readValue). We should revisit those converters for having a cleaner approach and to not distribute conversion of AggregateReference to components outside of MappingRelationalConverter/MappingJdbcConverter.

I was able to simply remove AggregateReferenceConverters. They weren't used anymore.
@schauder schauder closed this May 27, 2025
@schauder schauder assigned mp911de and unassigned schauder May 27, 2025
@schauder
Copy link
Contributor Author

Oops.
@schauder schauder reopened this May 27, 2025
schauder added 5 commits July 3, 2025 15:51
@schauder
1828-aggregate-ref-with-convertable - Prepare branch
fc4e20f
@schauder
Support AggregateReferences with custom id type.
fad6568 
Restructured reading conversion process into: - converting technology base types (JDBC Arrays). - standard and custom conversions. - module specific conversions (AggregateReference). Closes #1828 Original pull request #2062
@schauder
Process review comments.
49f26e0 
- Wrapped SqlException and rethrow it. - remove AggregateReferenceConverters completely. It was no longer used. Closes #1828 Original pull request #2062
@schauder
Simplified structure of MappingRelationalConverter.readValue().
a891485 
See #1828 Original pull request #2062
@schauder
Fixed writing AggregateRef with custom write target.
ef1a0cb 
The TODO mentioned in review was already resolved. Added the proper conversion step during writing. Fixed the wrong assumption in the test, that the database will return a Long. Original pull request #2062 See #1828
@schauder schauder force-pushed the issue/1828-aggregateref-new branch from 483808f to ef1a0cb Compare July 3, 2025 13:53
@schauder
Copy link
Contributor Author

schauder commented Jul 3, 2025

Ready for another round of review.
mp911de pushed a commit that referenced this pull request Jul 16, 2025
@schauder @mp911de
Support AggregateReferences with custom id type.
002acbc 
Restructured reading conversion process into: - converting technology base types (JDBC Arrays). - standard and custom conversions. - module specific conversions (AggregateReference). Closes #1828 Original pull request #2062
mp911de added a commit that referenced this pull request Jul 16, 2025
@mp911de
Polishing.
43f4a54 
Use Standard Exception handling for JDBC SQLException. Introduce easier and consistent mechanism to construct JdbcCustomConversions. Add tests and fix mocking setup. Fix typos. See #1828 Original pull request #2062
mp911de added a commit that referenced this pull request Jul 16, 2025
@mp911de
Polishing.
dcf349f 
Align R2dbcCustomConversions creation with configurer pattern. See #1828 Original pull request #2062
@mp911de mp911de added this to the 4.0 M4 (2025.1.0) milestone Jul 16, 2025
@mp911de
Copy link
Member

mp911de commented Jul 16, 2025

That's merged and polished now. For the time being, I suggest keeping that change for 4.0 only and if it proves to not create downstream issues, we can consider it for backporting.
@mp911de mp911de closed this Jul 16, 2025
@mp911de mp911de deleted the issue/1828-aggregateref-new branch July 16, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug A general bug

2 participants

@schauder @mp911de