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

generator param of @GeneratedValue is not ignored when using GenerationType.AUTO on db where this uses identity #2915

Closed
Incanus3 opened this issue Dec 8, 2022 · 5 comments · Fixed by #2935
Assignees
Labels
Milestone

Comments

@Incanus3
Copy link
Contributor

Incanus3 commented Dec 8, 2022

Expected behavior

When strategy = GenerationType.IDENTITY is used, the @SequenceGenerator annotation and the generator parameter of @GeneratedValue annotation is ignored, which is a good thing, because these don't actually make much sense when not using sequences. I think the behavior should be the same if you use strategy = GenerationType.AUTO on a db that chooses IDENTITY as the strategy.

Actual behavior

If you use GenerationType.AUTO on a db that chooses IDENTITY as the strategy (so basically any db except Oracle) the @SequenceGenerator annotation is correctly ignored, but the generator param of @GeneratedValue is not, resulting in the "No custom IdGenerator registered with name <specified generator name>" exception.

Steps to reproduce

Define an entity like this and use it in a db which chooses the IDENTITY strategy for GenerationType.AUTO

@Entity
@DbName("ea")
@Table(name = "T_OBJECT")
class EaObject(
    @Id
    @Column(name = "OBJECT_ID", nullable = false)
    @GeneratedValue(
        strategy = GenerationType.AUTO,
        generator = "t_object_sequence_generator"
    )
    @SequenceGenerator(
        name = "t_object_sequence_generator",
        sequenceName = "OBJECT_ID_SEQ",
    )
    var id: Long = 0,
)
Caused by: java.lang.RuntimeException: Error reading annotations for cz.sentica.qwazar.ea.core.entities.EaObject
	at io.ebeaninternal.server.deploy.parse.ReadAnnotations.readInitial(ReadAnnotations.java:31)
	at io.ebeaninternal.server.deploy.BeanDescriptorManager.createDeployBeanInfo(BeanDescriptorManager.java:1168)
	at io.ebeaninternal.server.deploy.BeanDescriptorManager.readEntityDeploymentInitial(BeanDescriptorManager.java:641)
	at io.ebeaninternal.server.deploy.BeanDescriptorManager.deploy(BeanDescriptorManager.java:290)
	at io.ebeaninternal.server.core.InternalConfiguration.<init>(InternalConfiguration.java:129)
	at io.ebeaninternal.server.core.DefaultContainer.createServer(DefaultContainer.java:104)
	at io.ebeaninternal.server.core.DefaultContainer.createServer(DefaultContainer.java:29)
	at io.ebean.DatabaseFactory.createInternal(DatabaseFactory.java:136)
	at io.ebean.DatabaseFactory.create(DatabaseFactory.java:85)
Caused by: java.lang.IllegalStateException: No custom IdGenerator registered with name t_object_sequence_generator
	at io.ebeaninternal.server.deploy.parse.AnnotationFields.readGenValue(AnnotationFields.java:469)
	at io.ebeaninternal.server.deploy.parse.AnnotationFields.initIdentity(AnnotationFields.java:162)
	at io.ebeaninternal.server.deploy.parse.AnnotationFields.readField(AnnotationFields.java:136)
	at io.ebeaninternal.server.deploy.parse.AnnotationFields.parse(AnnotationFields.java:62)
	at io.ebeaninternal.server.deploy.parse.ReadAnnotations.readInitial(ReadAnnotations.java:29)
	... 126 more
@Incanus3
Copy link
Contributor Author

Incanus3 commented Dec 8, 2022

I've already created a google group discussion for this here with some reasoning about why we need this kind of behavior, but I've noticed the discussions created after October 10th haven't gotten any answers and I also think this is a bug, because of the inconsistency between explicit IDENTITY strategy and the AUTOmatically chosen one.
If on the other hand you don't think this is a bug and don't want to change the behavior, can you tell me if there's a better way to support both identity- and sequence-based id assigning in the same entity? The only option I could think of is to create a @MappedSuperclass for each of our entities and have 2 subclasses with the db-specific id definition for each. Then we'd also need to duplicate all our DAOs in a similar manner.

rbygrave added a commit that referenced this issue Jan 17, 2023
…ignored when using GenerationType.AUTO on db where this uses identity
@rbygrave
Copy link
Member

If you use GenerationType.AUTO on a db that chooses IDENTITY ...

In the test it fails regardless of the DB Identity preference. That is, for GenerationType.AUTO and a non-empty generator value ebean is expecting the generator to specify a custom generator and when that isn't found it throws the exception then. This is way before it looks at the database platform preferred identity/sequence strategy.

That is, there is NO use of the database platform preferred identity/sequence at this stage - that happens much later in BeanDescriptorManager.setIdGeneration().

Really it looks like 2 paths to make it work:

  1. Just remove the generator = "t_object_sequence_generator" from @GeneratedValue
  2. Modify Ebean AnnotationFields.readGenValue() such that when a custom id generator is not found it does not throw the exception but instead does nothing. This then allows it to continue where it eventually gets to BeanDescriptorManager.setIdGeneration() which then sets the appropriate id generation from the database platform default.

Did you try just removing the generator = "t_object_sequence_generator" from @GeneratedValue?

@Incanus3
Copy link
Contributor Author

Hi and thanks for the reply. Yeah, it does indeed work if I remove the generator param completely and that's what we're using now, since we're not using ebean to db schema yet. But we want to start using ebean migrations in the future and then we'll need this param, because the schema has a non-standard sequence name, which we need to specify in the @SequenceGenerator annotation, which is referenced by this param.

@rbygrave
Copy link
Member

rbygrave commented Jan 19, 2023

has a non-standard sequence name, which we need to specify in the @SequenceGenerator annotation, which is referenced by this param.

Well, when using Ebean it isn't referenced by this param so the below works as we'd desire. However, with JPA it really suggests that it is referenced this way. So the fact that Ebean want's us to NOT have the generator referenced is kind-of-the-bug-here.

So this below without the generator = "t_object_sequence_generator" works:

@Entity
@Table(name = "T_OBJECT")
public class EaObject extends Model {

  @Id
  @Column(name = "OBJECT_ID", nullable = false)
  @GeneratedValue(
    strategy = GenerationType.AUTO
  )
  @SequenceGenerator(
    name = "t_object_sequence_generator",
    sequenceName = "FRED_OBJECT_ID_SEQ"
  )
  long id;
  final String name;
  ...
}

With a DB with SEQUENCE as it's preferred strategy we get:

create table T_OBJECT (
  OBJECT_ID                     bigint not null,
  name                          varchar(255),
  constraint pk_t_object primary key (OBJECT_ID)
);
create sequence FRED_OBJECT_ID_SEQ start with 1 increment by 1;

And to stress the point using THIS_NAME_IS_SILLY ... ebean isn't referencing it by name so this also works.

  @Id
  @Column(name = "OBJECT_ID", nullable = false)
  @GeneratedValue(
    strategy = GenerationType.AUTO
  )
  @SequenceGenerator(
    name = "THIS_NAME_IS_SILLY"
    ,sequenceName = "BOB_OBJECT_ID_SEQ"
  )
  long id;
create table T_OBJECT (
  OBJECT_ID                     bigint not null,
  name                          varchar(255),
  constraint pk_t_object primary key (OBJECT_ID)
);
create sequence BOB_OBJECT_ID_SEQ start with 1 increment by 1;

Noting that the JPA identity/sequence annotations are less than ideal* in a few ways which is why Ebean also has it's own @Identityso we can just do ...

  @Id
  @Column(name = "OBJECT_ID", nullable = false)
  @Identity(sequenceName = "KATIE_OBJECT_ID_SEQ")
  long id;
create table T_OBJECT (
  OBJECT_ID                     bigint not null,
  name                          varchar(255),
  constraint pk_t_object primary key (OBJECT_ID)
);
create sequence KATIE_OBJECT_ID_SEQ increment by 1;

* JPA identity/sequence annotations are less than ideal

  • We want to additionally control generated by default / generated always for identity columns
  • have the cache option for both identity and sequence
  • be able to define these options on the class itself as well as the @Id
  • imo the JPA annotations are not very intuitive.

In Review:

JPA annotations really suggest we need the @GeneratedValue(generator = ...) property to reference the sequence. Ebean will actually barf on this because it instead thinks it's trying to reference a custom IdGenerator (JPA GenerationType doesn't have a GenerationType.APPLICATION which would be nice and explicit).

The fix is to remove that check and let it flow through to then assign the database platform default identity/sequence etc.

rbygrave added a commit that referenced this issue Jan 19, 2023
…n using GenerationType.AUTO on db where this uses identity
@rbygrave rbygrave self-assigned this Jan 19, 2023
@rbygrave rbygrave added the bug label Jan 19, 2023
@rbygrave rbygrave added this to the 13.11.2 milestone Jan 19, 2023
rbygrave added a commit that referenced this issue Jan 19, 2023
#2915 - Failing test for - generator param of @GeneratedValue is not ignored when using GenerationType.AUTO on db where this uses identity
@Incanus3
Copy link
Contributor Author

Hi,
thanks for the detailed response, it really makes things clearer. I had no idea I don't need to reference the @SequenceGenerator name in the @GeneratedValue generator param, since we came to ebean from JPA where this was required (as you already mentioned). I also didn't know I can use the @Identity for sequences (though this is documented, but I kind of missed that) and I don't seem to be able to find the sequenceName param in the documentation at all. So yeah, this issue could be solved using means that ebean already provides, but thank you very much for fixing the JPA discrepancy anyway, hopefully it will prevent other people from running into the same issue.

Cheers again for this great ORM, it really makes my life as a developer much more pleasant.

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