Assigned id value is not passed into BeforeExecutionGenerator#generate() method when allowAssignedIdentifiers() is true and id has been assigned

Description

When I implement an id generator overriding allowAssignedIdentifiers method to return true as below:

public static class IdGenerator implements BeforeExecutionGenerator { @Override public Object generate( SharedSessionContractImplementor session, Object owner, Object currentValue, EventType eventType) { ... ... } @Override public EnumSet<EventType> getEventTypes() { return EnumSet.of( EventType.INSERT ); } @Override public boolean allowAssignedIdentifiers() { return true; }

When an id has been assigned to owner, to my surprise the currentValue is still null, not the assigned value, which means I have to work around this by the following code pattern:

@Override public Object generate( SharedSessionContractImplementor session, Object owner, Object currentValue, EventType eventType) { Object id = session.getEntityPersister( null, owner ).getIdentifier( owner ); if (id != null) { // id has been assigned } else { // not assigned, need to generate } // return id }

The above workaround seems unnecessary, tedious, error-prone (no clue on entity name, so null is used), defeating the purpose of currentValue parameter.

Seems we can modify upstream code logic to ensure that if the id has been assigned, the currentValue should not be null. Then we could enjoy the following code pattern intuitively:

@Override public Object generate( SharedSessionContractImplementor session, Object owner, Object currentValue, EventType eventType) { if (currentValue != null) { return currentValue; // assigned } // generate id then }

Activity

Gavin Kinglast week

Yeah, maybe … I guess I don’t have a strong opinion on that one.

Marco Belladellilast week
Edited

I agree, and actually I would go one step further and say that the currentValue parameter should always reflect the property’s state when generation occurs, regardless of what the allowAssignedIdentifiers method returns. Of course, if the latter does not return true, persisting entities with pre-assigned ids would fail as we would consider them as detached, so we can use that and avoid reading the entity property’s state unnecessarily.

Note: for non-identifier values, that is already the case. When inserting an entity, however, we always pass null as the current value for identifiers in #generate instead of reading any possible assigned values.

Gavin Kinglast week

Ah, yeah, I agree that’s not great.

WDYT? It should be ok to pass in the currently-assigned value when allowAssignedIdentifiers returns true, right?

Fixed

Details

Assignee

Reporter

Components

Fix versions

Affects versions

Priority

Created last week
Updated 3 days ago
Resolved 3 days ago

Flag notifications