jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
196 stars 58 forks source link

add comment and check to @Table and @Column #381

Closed gavinking closed 1 year ago

gavinking commented 2 years ago

I propose adding the new members comment and check to the @Table and @Column annotations, allowing the program to customize generated DDL with a check constraint and/or comment.

gavinking commented 2 years ago

A rough draft of this new feature is proposed here: https://github.com/jakartaee/persistence/pull/382

beikov commented 2 years ago

How about a @Check annotation that also allows naming the constraint? Can you also provide an example how check would be used with the @Table annotation? I'm having a hard time imagining how this could be useful without the @Check annotation, in which case the member should probably be Check[] checks() default {};

gavinking commented 2 years ago

How about a @Check annotation that also allows naming the constraint?

Ummmm ... how widely supported is that? I don't usually give names to check constraints. (And we don't have support for it in Hibernate.)

Can you also provide an example how check would be used with the @Table annotation?

WDYM? SQL has a column-level check clause, and a table-level check clause, for enforcing multiple-column constraints, e.g.

    create table books (
       id int4 not null,
        isbn varchar(13),
        published date not null,
        title varchar(100) not null,
        author_id int4 not null,
        primary key (id),
        check (length(title)>length(isbn))
    )

(Stupid example of course.)

beikov commented 2 years ago

Ummmm ... how widely supported is that? I don't usually give names to check constraints. (And we don't have support for it in Hibernate.)

AFAIU every constraint can be named and has an implicit name anyways. We don't support that yet in Hibernate, but maybe we should. If you have multiple constraints, it would be nice to give fixed names to be able to understand in a ConstraintViolationException which constraint failed in a programmed manner i.e. react in a UI to a violation of a certain constraint.

WDYM? SQL has a column-level check clause, and a table-level check clause, for enforcing multiple-column constraints, e.g.

Ok, don't mind me. I always added check constraints explicitly with a name or to columns directly, so I wasn't aware that one can specify a check constraint this way.

gavinking commented 2 years ago

the member should probably be Check[] checks() default {};

The problem with that is it's just soooo much uglier to use, for kinda limited benefit, it seems to me.

beikov commented 2 years ago

The problem with that is it's just soooo much uglier to use, for kinda limited benefit, it seems to me.

The only use case for this member is when creating check constraints that involve multiple columns. Is it really so common to do that? And even if it is very common, I don't think this is a big deal, but maybe others have different opinions.

gavinking commented 2 years ago

AFAIU every constraint can be named and has an implicit name anyways.

OK, yeah, sure, fine, so the syntax (CONSTRAINT name)? CHECK expression does seem to be standard.

gavinking commented 2 years ago

The only use case for this member is when creating check constraints that involve multiple columns.

I think the main issue is that a toplevel @Check annotation is ambiguous when placed on an entity with joined inheritance or a @SecondaryTable. Making it a member eliminates that ambiguity.

A much less important motivation is for check constraints on join columns, since @JoinColumn is repeatable.

gavinking commented 2 years ago

@beikov OK, I've updated the PR to introduce a @CheckConstraint annotation. This now works like the existing @UniqueConstraint and @Index annotations.