Skip to content

Supported nested conversions for AggregateReference.#2062

Closed
schauder wants to merge 5 commits intomainfrom
issue/1828-aggregateref-new
Closed

Supported nested conversions for AggregateReference.#2062
schauder wants to merge 5 commits intomainfrom
issue/1828-aggregateref-new

Conversation

@schauder
Copy link
Copy Markdown
Contributor

@schauder schauder commented May 23, 2025

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
Copy Markdown
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.

Comment thread ...ain/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java Outdated
Comment thread ...data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/MappingJdbcConverter.java
@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
Copy Markdown
Contributor Author

schauder commented May 27, 2025

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
Copy Markdown
Contributor Author

schauder commented May 27, 2025

Oops.

@schauder schauder reopened this May 27, 2025
mp911de
mp911de reviewed Jul 1, 2025
View reviewed changes
Comment thread ...ain/java/org/springframework/data/relational/core/conversion/MappingRelationalConverter.java Outdated
Comment thread ...g/springframework/data/jdbc/repository/JdbcRepositoryCrossAggregateHsqlIntegrationTests.java Outdated
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
Copy Markdown
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
Copy Markdown
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

Reviewers

@mp911de mp911de mp911de left review comments

Assignees

@mp911de mp911de

Labels

type: bug A general bug

Projects

None yet

Milestone

4.0 M4 (2025.1.0)

Development

Successfully merging this pull request may close these issues.

AggregateReference with custom datatypes can not be converted to simple type

3 participants

@schauder @mp911de @spring-projects-issues