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.

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

Left Join Fetch on Embeddable ElementColection fails & NamedQuery wit… #2269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Left Join Fetch on Embeddable ElementColection fails & NamedQuery wit… #2269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...sistence.core/src/main/java/org/eclipse/persistence/internal/sessions/UnitOfWorkImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3051,7 +3051,7 @@ public Object internalRegisterObject(Object object, ClassDescriptor descriptor,
return null;
}
if (descriptor.isDescriptorTypeAggregate()) {
throw ValidationException.cannotRegisterAggregateObjectInUnitOfWork(object.getClass());
return null;
}
Object registeredObject = checkIfAlreadyRegistered(object, descriptor);
if (registeredObject == null) {
Expand Down
13 changes: 13 additions & 0 deletions ...nce.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metamodel/ManagedTypeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
import org.eclipse.persistence.internal.security.PrivilegedAccessHelper;
import org.eclipse.persistence.internal.security.PrivilegedGetDeclaredField;
import org.eclipse.persistence.internal.security.PrivilegedGetDeclaredMethod;
import org.eclipse.persistence.internal.sessions.AbstractSession;
import org.eclipse.persistence.logging.AbstractSessionLog;
import org.eclipse.persistence.logging.SessionLog;
import org.eclipse.persistence.mappings.CollectionMapping;
Expand Down Expand Up @@ -1329,6 +1330,18 @@ protected void initialize() { // Future: Check all is*Policy() calls
}
}

public void preinitaliseMappings(AbstractSession session) {
for (DatabaseMapping mapping : getDescriptor().getMappings()) {
try {
mapping.preInitialize(session);
} catch (NullPointerException npe) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but NullPointerException handling in this context doesn't suite project coding style.
It looks like more some workaround.
Please add test there which is used as a descriptor and prevent any regression in the future.
Good candidate for a test is following Maven module/location.
https://github.com/eclipse-ee4j/eclipselink/tree/master/jpa/eclipselink.jpa.testapps/jpa.test.fetchgroups/src/test/java/org/eclipse/persistence/testing/tests/jpa/fetchgroups

Copy link
Author

Choose a reason for hiding this comment

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

Do you have a suggestion on how this should be handled instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Find where NPE happens and avoid this call by something like if (someObject != null) .....

Copy link
Member

Choose a reason for hiding this comment

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

well, the proper way is to figure out what is null and if it can be null - if yes adding a null check is appropriate and if not - which is more likely - something needs to be set by some other code on a different place

Copy link
Member

Choose a reason for hiding this comment

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

also from the stack trace - it looks like this happens with no weaving. Does it work with dynamic or static weaving on?

// A NPE gets thrown if the expected method is not present for the mapping
AbstractSessionLog.getLog().log(SessionLog.FINE, "Caught NPE when preinitializing database mapping",
npe.getMessage());
}
}
}

/**
* INTERNAL:
* Get the elementType directly from the class using a reflective method call
Expand Down
11 changes: 8 additions & 3 deletions ...tence.jpa/src/main/java/org/eclipse/persistence/internal/jpa/metamodel/MetamodelImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,13 @@ public void initialize(ClassLoader classLoader) {
}
}

//1 - process all non-mappedSuperclass types first, so we pick up attribute types
//2 - process mappedSuperclass types and lookup collection attribute types on inheriting entity types when field is not set
//1 - preinitalise all mappings so attribute types are set
//2 - process all non-mappedSuperclass types first so we pick up attribute types
//3 - process mappedSuperclass types and lookup collection attribute types on inheriting entity types when field is not set

for(ManagedTypeImpl<?> managedType : new ArrayList<>(managedTypes.values())) {
managedType.preinitaliseMappings(session);
}

/*
* Delayed-Initialization (process all mappings) of all Managed types
Expand All @@ -510,7 +515,7 @@ public void initialize(ClassLoader classLoader) {
managedType.initialize();
}

// 3 - process all the Id attributes on each IdentifiableType
// 4 - process all the Id attributes on each IdentifiableType
for(ManagedTypeImpl<?> potentialIdentifiableType : managedTypes.values()) {
if(potentialIdentifiableType.isIdentifiableType()) {
((IdentifiableTypeImpl<?>)potentialIdentifiableType).initializeIdAttributes();
Expand Down
Loading