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

Fix for issue 2211 #2289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ajaypaul-ibm
Copy link
Contributor

This PR contains a fix for issue #2211. The query now correctly includes all primary keys instead of just one.

@rfelcman
Copy link
Contributor

Sorry this fix can't be accepted as tests
EclipseLink PR build / Test on JDK 21 (pull_request) continuous-integration/jenkins/pr-merge are not passing please search test log outputs and modify Your code.

@ajaypaul-ibm ajaypaul-ibm force-pushed the 2211 branch 2 times, most recently from 62f6e1d to 1c8cdbf Compare November 8, 2024 06:58
Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

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

I'm missing there tests. See mainly JUnitJPQLFunctionsTest.java or with this alias generation JUnitJPQLJakartaDataNoAliasTest.java

@ajaypaul-ibm
Copy link
Contributor Author

Hi @rfelcman
We have a few questions regarding this issue and have sent you an email with the details. Whenever you have a chance, could you kindly take a look?
Thank you very much for your time and help!

@ajaypaul-ibm
Copy link
Contributor Author

ajaypaul-ibm commented Nov 12, 2024

Hi @rfelcman I went through the mail. Is @IdClass is not supported in EclipseLink?

@rfelcman
Copy link
Contributor

Hi @rfelcman I went through the mail. Is @IdClass is not supported in EclipseLink?

Of course @IdClass it's supported in EclipseLink. Simply search code. You will find there a lot of examples in the tests. It just doesn't work with JPQL function ID() with multiple @Ids.

@ajaypaul-ibm
Copy link
Contributor Author

Hi @rfelcman I have send an mail can you please check it out ?

@ajaypaul-ibm
Copy link
Contributor Author

ajaypaul-ibm commented Dec 9, 2024

Hi @rfelcman
I was able to identify the location in EclipseLink where accessors are being processed. Specifically, when an entity with @IdClass is encountered, it seems to fall into the last else case here: MetadataDescriptor.java#L1542.

To handle this scenario, I attempted to add logic to check if the entity is using @IdClass. Below is an example of the extra else if block I added:

boolean isIdClass = accessor.getJavaClass().isAnnotationPresent("jakarta.persistence.IdClass", accessor.getClassAccessor());
else if (isIdClass) {
    if (accessor.isId()) {
        // Logic to handle @IdClass goes here
    }
}

However, I’m unsure of the specific logic that needs to go inside the if block to correctly process an entity with @IdClass.
Could you provide any hints or guidance on how to proceed?
Thank you for your help.

@rfelcman
Copy link
Contributor

Hi @rfelcman I was able to identify the location in EclipseLink where accessors are being processed. Specifically, when an entity with @IdClass is encountered, it seems to fall into the last else case here: MetadataDescriptor.java#L1542.

To handle this scenario, I attempted to add logic to check if the entity is using @IdClass. Below is an example of the extra else if block I added:

boolean isIdClass = accessor.getJavaClass().isAnnotationPresent("jakarta.persistence.IdClass", accessor.getClassAccessor());
else if (isIdClass) {
    if (accessor.isId()) {
        // Logic to handle @IdClass goes here
    }
}

However, I’m unsure of the specific logic that needs to go inside the if block to correctly process an entity with @IdClass. Could you provide any hints or guidance on how to proceed? Thank you for your help.

I think, that MetadataDescriptor.java is too generic. Try


And another area where fix should be targeted is org.eclipse.persistence.internal.jpa.metadata.accessors.mappings.EmbeddedIdAccessor or org.eclipse.persistence.internal.jpa.metadata.accessors.mappings.DerivedIdClassAccessor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants