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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Supported nested conversions for AggregateReference. #2062

wants to merge 4 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 2 commits May 27, 2025 10:20
@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
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
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
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
No milestone
Development

Successfully merging this pull request may close these issues.

AggregateReference with custom datatypes can not be converted to simple type
2 participants
@schauder @mp911de