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

Native support for UUID class #152

Closed
lukasj opened this issue Jul 14, 2017 · 13 comments · Fixed by #319
Closed

Native support for UUID class #152

lukasj opened this issue Jul 14, 2017 · 13 comments · Fixed by #319
Labels

Comments

@lukasj
Copy link
Contributor

lukasj commented Jul 14, 2017

It would be really great if UUID would be a valid type for (at least PK) attributes. Certainly it can be provided by an application using an adapter class, but in the context of massively distributed environments it seems to be a common need of all cloud applications.

@lukasj
Copy link
Contributor Author

lukasj commented Jul 14, 2017

@mkarg Commented
I developed an AttributeConverter<UUID> and could contribute it to the RI.

@lukasj
Copy link
Contributor Author

lukasj commented Jul 22, 2017

@rbygrave Commented
Note that I don't see AttributeConverter as sufficient due to the differences in database support - native UUID type in Postgres etc and resulting DDL generation and binding differences. That is, in my opinion the ORM vendor really ought to deal with those specific details as well (been here, got the T-Shirt).

@lukasj
Copy link
Contributor Author

lukasj commented Aug 31, 2018

@mkarg
Copy link

mkarg commented Aug 31, 2018

@mkarg I was the original reporter.

gavinking added a commit to gavinking/persistence that referenced this issue May 5, 2021
@sebersole
Copy link
Contributor

sebersole commented May 5, 2021

Ideally we'd allow the user some way to specify the database storage for these. Maybe even a limited set like is done for enums:

@Basic
@UUID( STRING )
UUID someUuid;

The ability to specify the JDBC type code (and let the provider figure it out) would be good also:

@Basic
@UUID( Types.VARCHAR )
UUID someUuid;

@rbygrave
Copy link

rbygrave commented May 5, 2021

In terms of how UUID is stored [VARCHAR, BINARY, NATIVE + an optimised option] a point I'd like to add is that for myself I expect this storage choice to be more per persistence context / database than per entity property. That is, it is more usual/expected that all UUID for Postgres are stored natively vs all UUID with Oracle stored as BINARY etc.

That is, I'm not sure we need the storage type on a per property basis via a @UUID. Noting that from an Ebean perpective we'd say we do not need @UUID at all - it is more provider side configuration of how the Java UUID type is stored/mapped as a database column.

I'm not sure I've explained that well but the short version is I don't think we need @UUID( Types.VARCHAR ) for a specific entity property (but instead this is more a global provider config/type mapping).

Edited: Improve wording

@sebersole
Copy link
Contributor

Just like with @GeneratedValue#strategy or @Enumerated#value there will be a default/auto choice which will in fact do exactly what you suggest.

However, just because something is "more usual/expected" does not mean that is how everyone uses these things. I could make the same argument for example wrt id-generators. I could say that it is "more usual/expected" when running on Oracle to use a sequence; but not everyone does that for every id.

And that last bit ("for every id") is important - I might generally want to store UUID as a PGSQL UUID but for a particular attribute store it as a VARCHAR. That's not completely unreasonable. So you'd need a way to indicate, for this one attribute, use a different strategy

@rbygrave
Copy link

rbygrave commented May 6, 2021

for a particular attribute store it as a VARCHAR. That's not completely unreasonable.

Yes, fair point. I think this case is currently covered by either using a "custom named generator" via GeneratedValue.generator or a @Column mapping? Perhaps I am missing something? Not as elegant/nice/explicit as a @UUID(Types.VARCHAR).

@sebersole
Copy link
Contributor

@Column unfortunately won't work very well if I understand what you mean. Are you saying to somehow parse the String provided as @Column#columnDefinition and determine a "type code"?

@Id
@GeneratedValue
UUID id;

So far nothing here tells us how to store this thing. IMO this would trigger what you described above, having the provider decide (based on the database, etc).

Say I want to store that as a VARCHAR. Using @Column, I think you are saying we'd do something like:

@Id
@GeneratedValue
@Column( columnDefinition = "VARCHAR" )
UUID id;

Parsing strings is rarely straight-forward. E.g., what about

@Id
@GeneratedValue
@Column( columnDefinition = "VARCHAR(36)" )
UUID id;

Generator won't work either. Ultimately this is about how we convert the UUID to its db storage type. Generator simply generates the value. Not to mention, part of the original topic here is asking whether we should support UUID beyond just identifiers (aka, a @Basic attribute), which would not have a generator.

I guess it is valid to say that this conversion happens via an AttributeConverter, except the spec says you cannot apply a converter to the id.

@gavinking
Copy link
Contributor

@sebersole So I've been thinking this one over. At first blush I was inclined to agree with @rbygrave that this was more a dialect-level thing, but I think you've convinced me that's not quite right.

Am I right in thinking that the only realistic possibilities here are:

  • CHAR/VARCHAR,
  • BINARY, or
  • native database type like UUID?

If so, would it really be so bad to handle the problem via a JPA converter? i.e. one that converts the UUID to a String or byte[] array?

I mean, sure, I can totally see how using some sort of SQL type code as you suggest with @UUID( Types.VARCHAR ) would be very convenient, more convenient certainly than having to write a converter.

But on the other hand it's not like this is sort of ambiguity is specific to UUIDs. A feature like that would be convenient, for sure, but I guess I think that if JPA was going to offer something like that, it should offer it not just for UUIDs, but more generally for other types as well.

Perhaps it's true that the need is more acute with UUIDs than with other types, but that's not quite obvious (to me).

So then I ask myself: for other types, how do I handle the problem in JPA handle today? Well, I think I'm supposed to use a converter, right?

Well, usually, yes. But there are indeed two cases which look like exceptions to that:

  • @Temporal / @TemporalType, and
  • @Enumerated / @EnumeratedType.

Now, I view @Temporal as essentially-deprecated after the introduction of java.time types. Furthermore, this case is not precisely analogous because the purpose of @Temporal is really to disambiguate what sort of a thing a java.util.Date represents.

But @Enumerated is certainly analogous to this case, and does provide a precedent for introducing something like, uff, I dunno, @Universal + UUIDType.

But unless the need is really that acute, I guess I would still prefer to tell people to write a converter. (Assuming that really is a solution.)

@sebersole
Copy link
Contributor

@sebersole So I've been thinking this one over. At first blush I was inclined to agree with @rbygrave that this was more a dialect-level thing, but I think you've convinced me that's not quite right.

I actually do agree with @rbygrave that there is a dialect aspect to this. I just think that only comes in to play in the implicit/auto case. Just because I use PGQL does not mean I want to use its native UUID type. If we want to add UUID support and not let users control how that maps, then this auto approach is enough and providers can simply do what they think is best.

As we keep coming back to, the problem with handling this via AttributeConverter is that a converter cannot be applied to an identifier. A huge chunk of usage of UUID is going to be as an identifier. If not for this identifier restriction, an auto-applied converter would solve probably every use case and be super trivial.

To me, a more apt comparison, strange as it sounds, is @Lob. That annotation implies a database type as well as a transformation (if I apply it to a String e.g.).

FTR, the type code was really just a spit-ball illustration. Probably a dedicated JPA enum would be better. Something like:

enum UUIDStrategy {
    AUTO,
    STRING,
    BINARY
}

which is consistent with @Enumerated e.g.

Maybe we could "fudge" the converter restriction and have something like:

@interface UUID {
    Class<AttributeConverter<?,?>> conversion() default void.class;
}

@gavinking
Copy link
Contributor

gavinking commented May 7, 2021

Just because I use PGQL does not mean I want to use its native UUID type.

Sure. OTOH, not all capabilities need to be defined in the spec. So there's still a question of whether this needs to be completely portable.

As we keep coming back to, the problem with handling this via AttributeConverter is that a converter cannot be applied to an identifier.

OK, yeah, good point, sorry, I missed that wrinkle.

Maybe we could "fudge" the converter restriction

I wonder if the restriction itself could be weakened in some way.

gavinking added a commit to gavinking/persistence that referenced this issue Jan 14, 2022
gavinking added a commit to gavinking/persistence that referenced this issue Jan 14, 2022
gavinking added a commit to gavinking/persistence that referenced this issue Jan 14, 2022
gavinking added a commit to gavinking/persistence that referenced this issue Jan 14, 2022
gavinking added a commit to gavinking/persistence that referenced this issue Jan 14, 2022
gavinking added a commit to gavinking/persistence that referenced this issue Jan 14, 2022
lukasj pushed a commit that referenced this issue Jan 17, 2022
@lukasj
Copy link
Contributor Author

lukasj commented Jan 17, 2022

The PR for this has been merged. Should the improvement be needed, file a new issue, please.

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

Successfully merging a pull request may close this issue.

5 participants