Fix for issue 2211

[ajaypaul-ibm]

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

[rfelcman]

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.

[rfelcman]

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

[ajaypaul-ibm]

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]

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

[rfelcman]

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]

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

[ajaypaul-ibm]

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]

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

eclipselink/jpa/org.eclipse.persistence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metadata/accessors/classes/MappedSuperclassAccessor.java

Line 605 in 2dd631c

protected void initIdClass() {

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

[rfelcman]

I'm missing Unit/integration test like
https://github.com/eclipse-ee4j/eclipselink/pull/2278/files

[rfelcman]

https://github.com/eclipse-ee4j/eclipselink/pull/2287/files

[ajaypaul-ibm]

Hi @rfelcman I have added unit test case . Please check

[rfelcman]

After resolve code comments please rebase this PR against latest master and update copyright years in the file headers.

[rfelcman]

Maybe this part should be refactored into

        if (isEmbeddable(descriptor.getMappings())) {
            String idAttributeName = getIdAttributeNameByField(descriptor.getMappings(), primaryKeyFields.get(0));
            StateFieldPathExpression stateFieldPathExpression = new StateFieldPathExpression(
                    expression.getParent(), variableText + "." + idAttributeName);
            expression.setStateFieldPathExpression(stateFieldPathExpression);
            // Continue with created StateFieldPathExpression
            // It handle by ObjectBuilder booth @Id/primary key types (simple/composite)
            expression.getStateFieldPathExpression().accept(this);
        } else {
            for (DatabaseField primaryKeyField : primaryKeyFields) {
                String idAttributeName = getIdAttributeNameByField(descriptor.getMappings(), primaryKeyField);
                StateFieldPathExpression stateFieldPathExpression = new StateFieldPathExpression(
                        expression.getParent(), variableText + "." + idAttributeName);
                expression.setStateFieldPathExpression(stateFieldPathExpression);
                // Continue with created StateFieldPathExpression
                // It handle by ObjectBuilder booth @Id/primary key types (simple/composite)
                expression.getStateFieldPathExpression().accept(this);
            }
        }

[rfelcman]

Another comment: Maybe mentioned code should be optimized to remove some duplicitous calls.

[rfelcman]

(expression.getSelectExpression() instanceof IdExpression) is not correct pattern in this context. There should be new method with

    @Override
    public void visit(IdExpression expression) {
        multipleSelects = true;
        ...
    }

[ajaypaul-ibm]

@rfelcman What should be the content of this method ?

[rfelcman]

I don't know, check other public void visit( as examples.

[ajaypaul-ibm]

@rfelcman Do you mean to check whether the expression is an instance of IdExpression inside visit() method ?

[rfelcman]

@rfelcman Do you mean to check whether the expression is an instance of IdExpression inside visit() method ?

Yes. Main reason is eliminate || (expression.getSelectExpression() instanceof IdExpression) by new public void visit(IdExpression expression) { method which is called automaticaly by visitor pattern.

[rfelcman]

This test is not enough. Please add there asserts for a values.

[ajaypaul-ibm]

@rfelcman Shall I make the changes and update the PR ?

[rfelcman]

@rfelcman Shall I make the changes and update the PR ?

Yes this is why PR technology exists plus hooked test calls which will verify modified code.

[rfelcman]

I don't see any changes in the code -> no any reason to review it again.