[master] Bug 539822: Fixes #260 #261

jvissers · 2018-10-04T22:11:34Z

for #260

Signed-off-by: Jan Vissers [email protected]

…dSuperclasses Signed-off-by: Jan Vissers <[email protected]>

eclipsewebmaster · 2018-10-04T22:11:39Z

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=539822

eclipsewebmaster · 2018-10-04T22:11:40Z

Issue tracker reference:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=539822

dazey3 · 2018-10-05T16:48:21Z

I need a test to be added as part of this delivery. We try to not deliver changes without tests to demonstrate that there is an issue and that this change fixes it.

You were saying that this issue was uncovered during some testing you were doing for migrating from ECL 2.6 -> 2.7, right? If you have a test that fails, please add that test to the eclipselink.jpa.test.jse bucket. If you look at other tests in that bucket and how they are constructed, it should be fairly easy to see how you can create a test there as well. If you have any questions getting that working, please let me know.

lukasj · 2018-10-05T16:53:25Z

note that this change may affect static weaving, see full change for the context

jvissers · 2018-10-05T17:12:48Z

Yeah the tests I conducted were done using our test harness. It tried to get the test going in eclipselink.jpa.test.jse but unfortunately I didn't get that far. I mean - I had a pretty hard time getting all the required pieces together to compile the whole thing in the first place. So I guess what I'm saying is that: sure I want to come up with a test, but I first could do with some more detailed explanation of how to set this thing up locally.

On the comment about static weaving. I noticed that the weaver has changed to support weaving of mappedsuperclasses, which is great because before 2.7 we had to work around that. I don't see how a change in the MetaDataProject stage2 call affects static weaving.

dazey3 · 2018-10-05T17:13:23Z

@lukasj Ok, there is some stuff here that I don't quite follow (like the fact that there are two maps), but I have a couple suggestions:

processStage2() should just process both getMappedSuperclasses() and getMetamodelMappedSuperclasses(). This seems simple and safe enough given the use of a isProcessed() check.
Maybe MetadataProject.addMappedSuperclass() should be putting into m_metamodelMappedSuperclasses as well. Less in favor of this change without an understanding of why two maps exist, but seems like it would work given the change to MetadataProcessor from the original patch

dazey3 · 2018-10-05T17:19:11Z

@jvissers Are you saying you are having a hard time getting EclipseLink to build&compile locally? Or are you having a hard time getting your test to compile and run in the JSE bucket? We can definitely help in either of those issues

jvissers · 2018-10-05T17:50:03Z

I seem to run into issues compiling each and every bit. With the risk of being 'noisy' - this is the first issue I encounter.

build-dirs:
    [mkdir] Created dir: /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/classes

compile:
    [javac] Compiling 3 source files to /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/classes
    [javac] /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/src/org/eclipse/persistence/testing/tests/logging/slf4j/SLF4JLoggerTest.java:24: error: package org.eclipse.persistence.testing.framework.junit does not exist
    [javac] import org.eclipse.persistence.testing.framework.junit.LogTestExecution;
    [javac]                                                       ^
    [javac] /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/src/org/eclipse/persistence/testing/tests/logging/slf4j/SLF4JLoggerTest.java:25: error: package org.eclipse.persistence.testing.tests.junit.logging does not exist
    [javac] import org.eclipse.persistence.testing.tests.junit.logging.LogLevelHelper;
    [javac]                                                           ^
    [javac] /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/src/org/eclipse/persistence/testing/tests/logging/slf4j/SLF4JLoggerTest.java:41: error: cannot find symbol
    [javac]     @Rule public LogTestExecution logExecution = new LogTestExecution();
    [javac]                  ^
    [javac]   symbol:   class LogTestExecution
    [javac]   location: class SLF4JLoggerTest
    [javac] /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/src/org/eclipse/persistence/testing/tests/logging/slf4j/SLF4JLoggerTest.java:41: error: cannot find symbol
    [javac]     @Rule public LogTestExecution logExecution = new LogTestExecution();
    [javac]                                                      ^
    [javac]   symbol:   class LogTestExecution
    [javac]   location: class SLF4JLoggerTest
    [javac] /home/jvissers/Projects/github/eclipselink/eclipselink/foundation/eclipselink.extension.test/src/org/eclipse/persistence/testing/tests/logging/slf4j/SLF4JLoggerTest.java:85: error: cannot find symbol
    [javac]         final String defaultLevelString = LogLevelHelper.logIdToName(defaultLevel);
    [javac]                                           ^
    [javac]   symbol:   variable LogLevelHelper
    [javac]   location: class SLF4JLoggerTest
    [javac] 5 errors
```

jvissers · 2018-10-05T17:51:22Z

With respect to the jpa.test.jse - when I build that I get:

test-run:
    [junit] Running org.eclipse.persistence.jpa.test.version.Batch-With-Multiple-Tests
    [junit] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0 sec

BUILD FAILED
/home/jvissers/Projects/github/eclipselink/eclipselink/jpa/eclipselink.jpa.test.jse/antbuild.xml:105: Process fork failed.

jvissers · 2018-10-05T18:04:11Z

Okay that test issue might be related to the Java version I used by default.
When I run this with Java 11 I get:

test-run:
    [junit] Error occurred during initialization of boot layer
    [junit] java.lang.module.FindException: Module java.transaction not found
    [junit] Running org.eclipse.persistence.jpa.test.version.Batch-With-Multiple-Tests
    [junit] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0 sec
    [junit] Tests FAILED (crashed)

Let me try with Java 9 (downloading now) and I'll get back to you. Java 11 has removed all ee parts - so that might be causing this.

lukasj · 2018-10-05T18:16:15Z

You need jdk 8. 9 may work too but run tests different way than 8.

jvissers · 2018-10-05T18:22:03Z

jdk 8 gave me could not fork problem.
Now running with jdk 9 and getting further.

Now up against:

org/hamcrest/SelfDescribing

java.lang.NoClassDefFoundError: org/hamcrest/SelfDescribing
at java.base/java.lang.ClassLoader.defineClass1(Native Method)
at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1007)
at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:801)
at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:699)

I'm trying to piece together where you guys expect hamcrest-core and what the exact naming should be :-)

dazey3 · 2018-10-05T18:50:06Z

@jvissers So, I have hamcrest-core downloaded locally. Then, I have a file called build.properties in my users directory, with this as the content:

junit.lib=C:/Tools/junit/junit/4.12/junit-4.12.jar/:C:/Tools/junit/hamcrest-core-1.3.jar

jdbc.driver.jar=C:/Tools/drivers/mysql/MySQL Connector J/mysql-connector-java-5.1.32.jar
db.driver=com.mysql.jdbc.Driver
db.url=jdbc:mysql://localhost/sampleDB
db.user=user
db.pwd=password
db.platform=org.eclipse.persistence.platform.database.MySQLPlatform

It seems like I should make a wiki page on "How to build EclipseLink"...

lukasj · 2018-10-05T18:52:35Z

I think you should be able to replicate what travis does: https://github.com/eclipse-ee4j/eclipselink/blob/master/.travis.yml

jvissers · 2018-10-06T07:41:05Z

After positioning JAR files in various places (seems not all 3rd party libs are expected in the same place), installing mysql, creating the correct db, user and password for it - I'm now able to run the test suite. I will try and come up with a test case that illustrates the problem and solution. That is not trivial I think, because in this scenario - I have to work with a generated canonical metamodel class and then use that in a unit test. I'm not sure if I can do both from within the same unit test - without any trickery.

Do you have a canonical metamodel test that was supposed to cover this (already)?

dazey3 · 2018-10-08T14:38:01Z

@jvissers I see a test class in eclipselink.jpa.test.jse bucket that may help: org.eclipse.persistence.jpa.test.modelgen.TestProcessor

Also, the eclipselink.jpa.test bucket has org.eclipse.persistence.testing.tests.jpa.metamodel.MetamodelMetamodelTest which seems to contain some metamodel tests.

Hope this helps!

jvissers · 2018-10-09T09:41:00Z

I haven't been able to reproduce the changed behavior with respect to canonical metamodel in 2.7.3 with a vanilla based test. Now we do have customized Eclipselink quite a bit with respect to various things - so I'm not ruling out that that might have something to do with it. So I'm know also having another look into that.

What I see in 2.6.5 and 2.7.3 when I run a test that uses our code is that in Metadataproject::processStage2: the m_mappedSuperclasseAccessors has a size of 0 and the m_metamodelMappedSuperclasses actually has the information holding on to the metamodel information.

Can you enlighten me what the rationale is/was for changing this in 2.7.3?
So why are we now reading from m_mappedSuperclasseAccessors as opposed to m_metamodelMappedSuperclasses?

jvissers · 2018-10-09T10:06:48Z

Ok - so one of the things that we should be aware of here is that in our case we are not using a persistence.xml file to declare our JPA stuff. Rather we use Spring's abstraction over PersistenceUnitInfo which allows us to programmatically manipulate the pui. In here we are just including the leaf entities - so not MappedSuperclass annotated classes. Speculating; this looks like it might be causing the m_mappedSuperclasseAccessors to remain empty.

Now when I look into the code that builds the Eclipselink metamodel I do notice this bit that gets triggered:

/**
     * INTERNAL:
     * Add mapped superclass accessors to inheriting entities.
     * Add new descriptors for these mapped superclasses to the core project.
     */
    protected void addPotentialMappedSuperclass(MetadataClass metadataClass, boolean addMappedSuperclassAccessors) {
        // Get the mappedSuperclass that was stored previously on the project
        MappedSuperclassAccessor accessor = getProject().getMappedSuperclassAccessor(metadataClass);

        if (accessor == null) {
            // If the mapped superclass was not defined in XML then check for a
            // MappedSuperclass annotation unless the addMappedSuperclassAccessors
            // flag is false, meaning we are pre-processing for the canonical
            // model and any and all mapped superclasses should have been
            // discovered and we need not investigate this class further.
            if (addMappedSuperclassAccessors) {
                if (metadataClass.isAnnotationPresent(JPA_MAPPED_SUPERCLASS)) {
                    m_mappedSuperclasses.add(new MappedSuperclassAccessor(metadataClass.getAnnotation(JPA_MAPPED_SUPERCLASS), metadataClass, getDescriptor()));

                    // 266912: process and store mappedSuperclass descriptors on
                    // the project for later use by the Metamodel API.
                    getProject().addMetamodelMappedSuperclass(new MappedSuperclassAccessor(metadataClass.getAnnotation(JPA_MAPPED_SUPERCLASS), metadataClass, getProject()), getDescriptor());
                }
            }

Note that this adds a MappedSuperclassAccessor to m_mappedSuperclasses but that field is declared on the ClassAccessor (is this correct?), as opposed to the MappedSuperclassAccessor that gets added to the m_metamodelMappedSuperclasses which is actually a field on the MetadataProject.

Effectively this means - that the m_mappedSuperclasses in MetadataProject will always be empty for us (regardless of whether it is version 2.6.5 or 2.7.3). I checked that by putting a breakpoint on void addMappedSuperclass(MappedSuperclassAccessor mappedSuperclass) in MetadataProject and for us it is never hit.

dazey3 · 2018-10-09T15:53:07Z

@jvissers

we are not using a persistence.xml file to declare our JPA stuff. Rather we use Spring's abstraction over PersistenceUnitInfo which allows us to programmatically manipulate the pui. In here we are just including the leaf entities - so not MappedSuperclass annotated classes.

I won't pretend to know much about spring's integration with JPA, but I think you need to use either XML mappings or annotations for your managed classes:

JPA 2.1 Specification; 8.2.1:

    The object/relational mapping information can take the form of annotations 
on the managed persistence classes included in the persistence unit, an orm.xml file 
contained in the META-INF directory of the root of the persistence unit, one or more 
XML files on the classpath and referenced from the persistence.xml file, or a 
combination of these.

Effectively this means - that the m_mappedSuperclasses in MetadataProject will always be empty for us (regardless of whether it is version 2.6.5 or 2.7.3)

I thought this worked for you on 2.6.5? How doesm_metamodelMappedSuperclasses get populated for you on 2.6.5 without XML mappings or annotated @MappedSuperclass?

jvissers · 2018-10-09T16:29:18Z

Actually you can build a EntityManagerFactory programmatically (and we do) - by leveraging the JPA Specification's PersistenceProvider method called public EntityManagerFactory createContainerEntityManagerFactory(PersistenceUnitInfo info, Map map).

It is working in 2.6.5 - because in that version the processStage2 is looking at m_metamodelMappedSuperclasses which will be populated from the method addPotentialMappedSuperclass which I referred to in an earlier comment.

See:

// 266912: process and store mappedSuperclass descriptors 
// the project for later use by the Metamodel API.
getProject().addMetamodelMappedSuperclass(new MappedSuperclassAccessor(metadataClass.getAnnotation(JPA_MAPPED_SUPERCLASS), metadataClass, getProject()), getDescriptor());

…dSuperclasses Signed-off-by: Jan Vissers <[email protected]>

…_master

dazey3 · 2018-10-09T19:40:38Z

Actually you can build a EntityManagerFactory programmatically (and we do) - by leveraging the JPA Specification's PersistenceProvider method called public EntityManagerFactory createContainerEntityManagerFactory(PersistenceUnitInfo info, Map map).

well, yes, you can use PersistenceProvider.createContainerEntityManagerFactory()... but unless you are writing a container implementation, I'm not sure why you'd want to... and in any case, this doesn't mean you don't have to annotate your @MappedSuperclass classes or use an orm.xml metadata mapping file.

It is working in 2.6.5 - because in that version the processStage2 is looking at m_metamodelMappedSuperclasses which will be populated from the method addPotentialMappedSuperclass which I referred to in an earlier comment.

The only way I see m_metamodelMappedSuperclasses being populate through ClassAccessor.addPotentialMappedSuperclass() is either:

m_mappedSuperclasseAccessors contains a matching MappedSuperclassAccessor
m_mappedSuperclasseAccessors doesnt contain a matching MappedSuperclassAccessor, but the class is annotated

Perhaps I misunderstand (wouldnt be the first time lol), but number 1 is out cause the whole issue is that getMappedSuperclasses() (iow m_mappedSuperclasseAccessors) is empty, right? Then number 2 is out cause the class isn't annotated @MappedSuperclass?

jvissers · 2018-10-09T19:43:56Z

I've added a test that will succeed with the fix I've proposed but will fail with current master (2.7.3). Mind you that we have customized Eclipselink quite extensively to meet our application architecture's requirements and programming model (which takes inspiration from DDD). I'll let you process the test first and can add more background later if that is necessary.

jvissers · 2018-10-09T19:51:12Z

To answer your questions

well, yes, you can use PersistenceProvider.createContainerEntityManagerFactory()... but unless you are writing a container implementation

We are not writing a container implementation, but using Spring's https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/orm/jpa/persistenceunit/PersistenceUnitPostProcessor.html. This along with other EL specific customizations enables us to create a rich domain model

Then number 2 is out cause the class isn't annotated @MappedSuperclass

From a JPA mapping perspective everything is okay.

jvissers · 2018-10-09T20:19:50Z

Sorry for my ignorance - but can you please explain whether the build (tests) run against the 'fixed' version of eclipselink or the one that is currently in the master branch? Asking because locally when I run the testsuite against the fixed eclipselink version all is green.

lukasj · 2018-10-09T20:43:52Z

...link.jpa.test.jse/src/org/eclipse/persistence/jpa/test/canonical/TestCanonicalMetaModel.java

+        properties.setProperty("javax.persistence.jdbc.driver", "com.mysql.jdbc.Driver");
+        properties.setProperty("javax.persistence.jdbc.url", "jdbc:mysql://localhost/user");
+        properties.setProperty("javax.persistence.jdbc.user", "user");
+        properties.setProperty("javax.persistence.jdbc.password", "password");


you want to read these from the system, see https://github.com/eclipse-ee4j/eclipselink/blob/master/jpa/eclipselink.jpa.test.jse/antbuild.xml#L110-L113

…_master Signed-off-by: Jan Vissers <[email protected]>

…_master

dazey3 · 2018-10-10T02:43:28Z

...clipselink.jpa.test.jse/src/org/eclipse/persistence/jpa/test/canonical/DomainInterface_.java

+import javax.persistence.metamodel.SingularAttribute;
+import javax.persistence.metamodel.StaticMetamodel;
+
+@Generated(value = "Simulated Generation", date = "2018-10-09T11:24:45")


Try to avoid using try the javax.annotation.processing.Generated annotation unless this is specifically needed for the test (I don't think it is).

Reason for this is that I'm trying to mimic the actual scenario at our end. We have a customized annotation processor that generates JPA canonical metamodel classes. You are (probably) right that the javax.annotation.processing.Generated here doesn't serve a (technical) purpose - it is more of a descriptive thing.

beware of the difference between javax.annotation.Generated which is in your import, and javax.annotation.processing.Generated you are referring to in your comment - they have different requirements on what exactly to put on the compiler's module-path for JDK 9+

I will take it out and push later today. Do you guys have any thoughts already on the actual test case and what it seems to be exposing?

…_master Signed-off-by: Jan Vissers <[email protected]>

…_master

jvissers · 2018-10-13T22:43:59Z

Hi, I'm curious about the status of this now. Can you please let us know?

jvissers · 2018-10-31T16:50:15Z

Would it be possible to get an update for this issue?

lukasj · 2018-12-18T18:47:02Z

yup, this breaks fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=466271

lukasj

Actually no, works fine. So looks good to me. Remaining thing is failing IP validation - did you signed https://www.eclipse.org/legal/ECA.php already? We cannot accept anything without that

jvissers · 2018-12-18T21:15:45Z

Just signed it.

lukasj · 2018-12-18T21:28:19Z

Hopefully last request(s) - can you force push this branch again to trigger ip-validation hook (or rebase against master...), please?

…_master Signed-off-by: Jan Vissers <[email protected]>

jvissers · 2018-12-18T21:56:31Z

Man this is painful! ip-validation still failing?!

lukasj · 2018-12-19T13:33:29Z

yes but there exists a way I can validate your account myself and it's OK now (apart from getting 'This user is missing from the ECA LDAP group' warning. Will see if the problem persists after 10minutes...)

Now waiting for travis to finish test run. I don't expect bad surprise, so it is likely I'll merge this to master later today. Will you prepare backport to 2.7 branch then, please?

Thanks!

Signed-off-by: Jan Vissers <[email protected]>

Bug 539822** - MetadataProject processStage2 to use getMetamodelMappe…

8f75eee

…dSuperclasses Signed-off-by: Jan Vissers <[email protected]>

jvissers mentioned this pull request Oct 4, 2018

Eclipselink MetaDataProject uses the wrong field in processStage2 #260

Closed

jvissers changed the title ~~master Bug 539822: Fixes #260~~ [master] Bug 539822: Fixes #260 Oct 4, 2018

jvissers added 2 commits October 9, 2018 21:26

Bug 539822** - MetadataProject processStage2 to use getMetamodelMappe…

f88a85a

…dSuperclasses Signed-off-by: Jan Vissers <[email protected]>

Merge branch '260_master' of github.com:jvissers/eclipselink into 260…

00570c7

…_master

lukasj reviewed Oct 9, 2018

View reviewed changes

jvissers added 2 commits October 9, 2018 23:49

Merge branch '260_master' of github.com:jvissers/eclipselink into 260…

0d43137

…_master Signed-off-by: Jan Vissers <[email protected]>

Merge branch '260_master' of github.com:jvissers/eclipselink into 260…

f310d62

…_master

dazey3 reviewed Oct 10, 2018

View reviewed changes

jvissers added 2 commits October 10, 2018 13:55

Merge branch '260_master' of github.com:jvissers/eclipselink into 260…

2d3ec76

…_master Signed-off-by: Jan Vissers <[email protected]>

Merge branch '260_master' of github.com:jvissers/eclipselink into 260…

52f5e4a

…_master

lukasj reviewed Dec 18, 2018

View reviewed changes

Merge branch '260_master' of github.com:jvissers/eclipselink into 260…

a5d4907

…_master Signed-off-by: Jan Vissers <[email protected]>

lukasj merged commit 52dcff1 into eclipse-ee4j:master Dec 19, 2018

lukasj pushed a commit to lukasj/eclipselink that referenced this pull request Dec 20, 2018

[master] Bug 539822: Fixes eclipse-ee4j#260 (eclipse-ee4j#261)

07ba867

Signed-off-by: Jan Vissers <[email protected]>

lukasj mentioned this pull request Dec 20, 2018

[2.7] Bug 539822: Fixes #260 (#261) #337

Merged

lukasj added a commit that referenced this pull request Dec 20, 2018

[master] Bug 539822: Fixes #260 (#261) (#337)

8e3d9ed

Signed-off-by: Jan Vissers <[email protected]>

OndroMih mentioned this pull request Aug 6, 2024

Fix for JPQL queries where identifier clashes with function #2218

Merged

[master] Bug 539822: Fixes #260 #261

[master] Bug 539822: Fixes #260 #261

Conversation

jvissers commented Oct 4, 2018 • edited by dazey3 Loading

eclipsewebmaster commented Oct 4, 2018

eclipsewebmaster commented Oct 4, 2018

dazey3 commented Oct 5, 2018 • edited Loading

lukasj commented Oct 5, 2018

jvissers commented Oct 5, 2018

dazey3 commented Oct 5, 2018 • edited Loading

dazey3 commented Oct 5, 2018

jvissers commented Oct 5, 2018

jvissers commented Oct 5, 2018

jvissers commented Oct 5, 2018

lukasj commented Oct 5, 2018

jvissers commented Oct 5, 2018

dazey3 commented Oct 5, 2018

lukasj commented Oct 5, 2018

jvissers commented Oct 6, 2018

dazey3 commented Oct 8, 2018

jvissers commented Oct 9, 2018

jvissers commented Oct 9, 2018

dazey3 commented Oct 9, 2018

jvissers commented Oct 9, 2018 • edited Loading

dazey3 commented Oct 9, 2018

jvissers commented Oct 9, 2018

jvissers commented Oct 9, 2018 • edited Loading

jvissers commented Oct 9, 2018

lukasj Oct 9, 2018

Choose a reason for hiding this comment

dazey3 Oct 10, 2018

Choose a reason for hiding this comment

jvissers Oct 10, 2018

Choose a reason for hiding this comment

lukasj Oct 10, 2018

Choose a reason for hiding this comment

jvissers Oct 10, 2018

Choose a reason for hiding this comment

jvissers commented Oct 13, 2018

jvissers commented Oct 31, 2018

lukasj commented Dec 18, 2018 • edited Loading

lukasj left a comment

Choose a reason for hiding this comment

jvissers commented Dec 18, 2018

lukasj commented Dec 18, 2018

jvissers commented Dec 18, 2018

lukasj commented Dec 19, 2018

jvissers commented Oct 4, 2018 •

edited by dazey3

Loading

dazey3 commented Oct 5, 2018 •

edited

Loading

dazey3 commented Oct 5, 2018 •

edited

Loading

jvissers commented Oct 9, 2018 •

edited

Loading

jvissers commented Oct 9, 2018 •

edited

Loading

lukasj commented Dec 18, 2018 •

edited

Loading