TCK should not redefine table on single table inheritance subclass #644
Comments
[jakartaee/persistence#644] Don't redefine table on single table inheritance subclass
|
@lukasj should we accept this challenge and exclude the test from Persistence 3.1? |
|
Failing tests are: |
|
I wonder if another JPA/Persistence Specification team committer could accept this challenge? @gavinking it looks to me like it would be ambiguous to specify @table on both classes as which table would be used if they specify different values? This challenge has been outstanding since July and should be accepted regardless based on the time that has gone by. |
Why the error is triggered? Sounds like an error to me because it only repeats an information which is already known and which looks to be in no conflict with the definition - a warning, OK, but an error? oh, come on...
depends on chosen inheritance strategy - table per concrete class (which is not mandatory for providers to support) vs single table per class, but the inheritance strategy tested here is the latter one. Anyway, one is supposed to be able to change the inheritance strategy/definition anywhere in the inheritance tree |
|
A TCK is meant to test things explicitly discussed in a specification. This scenario is not. Therefore the TCK should not be testing it. We can quibble about what the outcome should be, but that should be explicitly defined in the spec if the TCK is going to enforce it. |
How does this answer my question? |
|
I only see one "question" in your reply -
Assuming that's the question you mean, I guess I'd answer 2 things -
I'd in turn ask you what's the point/win in supporting this the way you assert? Just to duplicate information for no reason? |
|
Look at it like this... as a user, you define these 2 annotations. Clearly you expect them to do something. So why would a provider just ignore it? Wouldn't you want to know that something will not work the way you think it will? |
|
https://jakarta.ee/committees/specification/tckprocess/ speaks to this case I think which is why I thought it is valid:
|
|
The 3 challenged tests info from https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/jpa/core/inheritance/abstractentity/Client.java: /*
/*
/*
I don't see I really hope that we can replace the test assertions in the future with web links or something easier to deal with (all we need to do is search the TCK source for the assertion id but I don't have time to look at the moment). |
|
My first response started with Why the error is triggered? And that is the question I was referring to - I believe we can agree on the fact that a question mark clearly denotes the question. But I can rephrase it for sure - what is the reason/motivation behind treating this as an user error? Can the spec do something to improve in this regard?
All I'm trying to do is to understand the reason you believe the behaviour is wrong in order to be able to either accept or reject this and/or eventually propose an improvement to the spec, so the same problem can be avoided in other parts of the spec, should there be any. |
|
So I guess we all agree that the following should always be rejected: @Entity @Table("Super")
class Super { ... }
@Entity @Table("Sub")
class Sub extends Super{ ... }@sebersole argues that, in addition, this should be rejected: @Entity @Table("Super")
class Super { ... }
@Entity @Table("Super")
class Sub extends Super{ ... }since, while it's not clearly "incorrect", it's at best redundant noise. I don't personally really have an opinion on this; I can see reasonable arguments both ways. But I do agree that the TCK is not permitted to test things which are not clearly required by the spec, and after searching, I don't see anywhere that this requirement is implied, not even by means of "penumbras and emanations". We just don't say much about the
and I don't even think the spec defines the term primary table. |
You edited your comment, so I'm not sure. Maybe you added the question mark, maybe I just missed it and saw the second. You asked about a question so perhaps I just saw only the one. Again, not sure. Anyway, both questions are roughly the same.
I'm not saying the behavior is right or wrong today. I'm saying its undefined, which means each provider can handle it however they see fit. The spec does not say anything about this, so the test should be exluded or removed or fixed (to remove the extra table annotation). Scott was kind enough to paste the exact quote about challenges and this clearly falls under "a test asserts requirements over and above that of the specification". Now if you mean why do I find it conceptually wrong...
The generalized reason I made this change in Hibernate is because I have been improving validation of annotations to include more checks for annotations that don't make sense in different scenarios. Some of these we have already incorporated directly into 3.2 (e.g. "2.2.1 Persistent Attribute Type"). Here, as I stated earlier, the second table annotation is unnecessary. It introduces duplication, code duplication being bad for all the normal reasons it is bad. In fact, I'd argue that the spec should be explicit about the expectation of
[1] For mapped-superclass, it does say "A class designated as a mapped superclass has no separate table defined for it". That's not quite the same as explicitly stating |
|
To be honest, I tend to think that the spec currently requires inclusion of the given that the spec in 11.1.51 says:
and as I see it the there is also section 11.2.1.1 saying By default, a table is created for every top-level entity... regardless of what the by default means For this reason, I:
|
|
... but I can be completely wrong in my argument, of course ... |
|
We can certainly have different opinions, that's fine. But, as we seem to agree, the spec is not at all clear here. So thanks for accepting the challenge. |
…r TCK challenge jakartaee/persistence#644 Signed-off-by: Scott Marlow <[email protected]>
TCK should not redefine table on single table inheritance subclass for TCK challenge jakartaee/persistence#644
|
For reference, a Hibernate ORM user recently asked about how to define an index for a subclass field column, which would more or less require that the |
[jakartaee/persistence#644] Don't redefine table on single table inheritance subclass
Currently both
com.sun.ts.tests.jpa.core.inheritance.abstractentity.FullTimeEmployeeandcom.sun.ts.tests.jpa.core.inheritance.abstractentity.Employeehave the annotation@Tablewith the same contents, thoughFullTimeEmployeeextendsEmployee. The annotation onFullTimeEmployeeis unnecessary, but triggers a new validation error in Hibernate ORM.There is no clarification in the specification how this should behave and hence the TCK should not rely on how a provider treats this case.
Please exclude the test from the TCK. I will provide a PR with a fix.
The text was updated successfully, but these errors were encountered: