search

jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
199 stars 59 forks source link

TCK should not redefine table on single table inheritance subclass #644

Open beikov opened 1 month ago

beikov commented 1 month ago

Currently both com.sun.ts.tests.jpa.core.inheritance.abstractentity.FullTimeEmployee and com.sun.ts.tests.jpa.core.inheritance.abstractentity.Employee have the annotation @Table with the same contents, though FullTimeEmployee extends Employee. The annotation on FullTimeEmployee is 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.

scottmarlow commented 3 days ago

@lukasj should we accept this challenge and exclude the test from Persistence 3.1?

scottmarlow commented 3 days ago

Failing tests are:

com/sun/ts/tests/jpa/core/inheritance/abstractentity/Client.java#abstractEntityTest1_from_standalone                                                 Failed. Test case throws exception: com.sun.ts.lib.harness.EETest$Fault: Setup failed:
com/sun/ts/tests/jpa/core/inheritance/abstractentity/Client.java#abstractEntityTest2_from_standalone                                                 Failed. Test case throws exception: com.sun.ts.lib.harness.EETest$Fault: Setup failed:
com/sun/ts/tests/jpa/core/inheritance/abstractentity/Client.java#abstractEntityTest3_from_standalone                                                 Failed. Test case throws exception: com.sun.ts.lib.harness.EETest$Fault: Setup failed:
scottmarlow commented 2 days ago

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.

lukasj commented 2 days ago

The annotation on FullTimeEmployee is unnecessary, but triggers a new validation error in Hibernate ORM.

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...

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?

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

sebersole commented 2 days ago

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.

lukasj commented 2 days ago

A TCK is meant to test things explicitly discussed in a specification. This scenario is not. Therefore the TCK should not be testing it.

How does this answer my question?

sebersole commented 2 days ago

I only see one "question" in your reply -

OK, but an error?

Assuming that's the question you mean, I guess I'd answer 2 things -

  1. Again, point me to where this is covered in the spec. If you can't, it should not be asserted in the TCK (period).
  2. In Hibernate we've been trying to be much better about validating bad annotations. I call this a bad mapping.

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?

sebersole commented 2 days ago

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?

scottmarlow commented 2 days ago

https://jakarta.ee/committees/specification/tckprocess/ speaks to this case I think which is why I thought it is valid:

The following scenarios are considered in scope for test challenges:

  • Claims that a test assertion conflicts with the specification.
  • Claims that a test asserts requirements over and above that of the specification.
  • Claims that an assertion of the specification is not sufficiently implementable.
  • Claims that a test is not portable or depends on a particular implementation.
scottmarlow commented 2 days ago

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 @Table annotation mentioned in the ^ description but I didn't read the referenced assertions which could be done to understand the test writers perspective on why they wrote the test. Unless I'm missing something, I think they just casually included @Table twice without considering (updated) whether the Specification allows or doesn't allow that.

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).

lukasj commented 2 days ago

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?

I'd in turn ask you what's the point/win in supporting this the way you assert?

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.

gavinking commented 2 days ago

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 @Table annotation at all, beyond:

The Table annotation specifies the primary table for the annotated entity.

and I don't even think the spec defines the term primary table.

sebersole commented 2 days ago

I believe we can agree on the fact that a question mark clearly denotes the question.

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.

what is the reason/motivation behind treating this as an user error? All I'm trying to do is to understand the reason you believe the behaviour is wrong

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...

eventually propose an improvement to the spec... Can the spec do something to improve in this regard?

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 @Table in regards to inheritance hierarchies

[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 @Table is not allowed, but if we all agree that is the implication then fine.

lukasj commented 1 day ago

To be honest, I tend to think that the spec currently requires inclusion of the @Table in the definition of the PartTimeEmployee entity - or in general in every entity in an inheritance hierarchy - instead of its removal from the FullTimeEmployee one:

given that the spec in 11.1.51 says:

If no Table annotation is specified for an entity class, the default values defined in Table 47 apply.

and as I see it the PartTimeEmployee is an entity class and has no @Table, therefore its table should default to the Entity name which, as defined by the @Entity, is defaulted to the name of the class, which means to the @Table(name="PartTimeEmployee") and that is clearly something not only the test does not expect.

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 PartTimeEmployee is not a root entity, so the fact that a table is not being generated is correct

For this reason, I:

lukasj commented 1 day ago

... but I can be completely wrong in my argument, of course ...

sebersole commented 1 day ago

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.