Avoid data that could lead to ambiguous column label usage

[beikov]

See jakartaee/persistence#350 for further discussion

Fixes Issue
Specify the issue (link) that is solved with this pull request.

Related Issue(s)
Specify any related issue(s) links.

Describe the change
A clear and concise description of the change.

Additional context
Add any other context about the problem here.

CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @shighbar @gthoman @brideck @scottmarlow

[scottmarlow]

I see nothing wrong with the change myself. We would like to merge this change soon if the test truly needs it but first please could I get some help with understanding if this change is needed even though SqlResultSetMappings is specified? See more details below...

Should the the @SqlResultSetMappings in Order1 have avoided the need for this change?

Or is SqlResultSetMappings (click here for spec text) for something else?

Note that SqlResultSetMappings is also mentioned in section 3.10.16.1. Returning Managed Entities from Native Queries which was discussed in the issue but I didn't see SqlResultSetMappings mentioned in the issue and wonder if the SqlResultSetMappings should of helped.

[beikov]

Changing the data will uncover that the test is "wrong", but for further information on that, see jakartaee/persistence#350.

I tested this on EclipseLink and it will produce "wrong" results. The problem is that the test goes against the wording in 3.10.16.1, because the result label id appears twice in the result set metadata of the test. To fix that, the query and the SqlResultSetMapping have to be adapted:

@SqlResultSetMapping(name = "Order1ItemResults", entities = {
        @EntityResult(entityClass = com.sun.ts.tests.jpa.core.annotations.nativequery.Order1.class, fields = {
            @FieldResult(name = "id", column = "OID"),
            @FieldResult(name = "totalPrice", column = "OPRICE"),
            @FieldResult(name = "item", column = "OITEM") }) }),
        @EntityResult(entityClass = com.sun.ts.tests.jpa.core.annotations.nativequery.Item.class) }),

q = getEntityManager().createNativeQuery(
          "Select o.\"ID\" OID, o.\"TOTALPRICE\" OPRICE, "
              + " o.\"FK1_FOR_ITEM\" OITEM, i.\"ID\", i.\"ITEMNAME\" from \"ORDER1\" o, \"ITEM\" i "
              + "WHERE (o.\"TOTALPRICE\" > 140) AND (o.\"FK1_FOR_ITEM\" = i.\"ID\")",
          "Order1ItemResults").getResultList();

In fact, Order3ItemResults and nativeQueryTest3 do exactly that. So maybe nativeQueryTest1 should just be changed to a negative test i.e. assert an exception is thrown?

[scottmarlow]

Changing the data will uncover that the test is "wrong", but for further information on that, see eclipse-ee4j/jpa-api#350.

I tested this on EclipseLink and it will produce "wrong" results. The problem is that the test goes against the wording in 3.10.16.1, because the result label id appears twice in the result set metadata of the test. To fix that, the query and the SqlResultSetMapping have to be adapted:

@SqlResultSetMapping(name = "Order1ItemResults", entities = {
        @EntityResult(entityClass = com.sun.ts.tests.jpa.core.annotations.nativequery.Order1.class, fields = {
            @FieldResult(name = "id", column = "OID"),
            @FieldResult(name = "totalPrice", column = "OPRICE"),
            @FieldResult(name = "item", column = "OITEM") }) }),
        @EntityResult(entityClass = com.sun.ts.tests.jpa.core.annotations.nativequery.Item.class) }),

q = getEntityManager().createNativeQuery(
          "Select o.\"ID\" OID, o.\"TOTALPRICE\" OPRICE, "
              + " o.\"FK1_FOR_ITEM\" OITEM, i.\"ID\", i.\"ITEMNAME\" from \"ORDER1\" o, \"ITEM\" i "
              + "WHERE (o.\"TOTALPRICE\" > 140) AND (o.\"FK1_FOR_ITEM\" = i.\"ID\")",
          "Order1ItemResults").getResultList();

Thanks for explaining, the example makes sense!

In fact, Order3ItemResults and nativeQueryTest3 do exactly that. So maybe nativeQueryTest1 should just be changed to a negative test i.e. assert an exception is thrown?

To explore that, the nativeQueryTest1 test assertionIDs from
JavaDoc + SPEC should be examined to understand what the test is supposed to validate exactly:

PERSISTENCE:SPEC:736

The SQL query facility is intended to provide support for those cases where it is necessary to use the native SQL of the target database in use (and/or where Jakarta Persistence QL cannot be used). Native SQL queries are not expected to be portable across databases. 

PERSISTENCE:SPEC:1009

The SqlResultSetMapping annotation is used to specify the mapping of the result of a native SQL query or stored procedure. 

PERSISTENCE:JAVADOC:199 (returned EntityResult[], for jakarta.persistence.SqlResultSetMapping.entities)

Specifies the result set mapping to entities.

PERSISTENCE:JAVADOC:64

The class of the result.

PERSISTENCE:JAVADOC:41

Create an instance of Query for executing a native SQL query.

Since the current Spec text does not make it clear that an exception is expected for the negative test case idea (unless I am missing something), I do not think we need the TCK test to validate that an exception is thrown for the current test.

More background on test assertions in general are listed on https://github.com/eclipse-ee4j/jakartaee-tck/wiki/Guide-to-adding-Specification-Assertions

From my reading of the test change and the detail given about why the test needs to change, I think we should merge the change. Will leave it open for another day to see if we get more feedback first.

Thanks for the help and contribution @beikov

[beikov]

Note that this data change would simply show that the test is broken and would clearly show to implementers that their implementations (Hibernate, EclipseLink) just can't make this magic happen that the test seemed to expect.

IMO, the test should simply be disabled or changed in a way such that it behaves like nativeQueryTest3

[scottmarlow]

Note that this data change would simply show that the test is broken and would clearly show to implementers that their implementations (Hibernate, EclipseLink) just can't make this magic happen that the test seemed to expect.

IMO, the test should simply be disabled or changed in a way such that it behaves like nativeQueryTest3

Could you paste in here what the updated nativeQueryTest1 method should look like after it is modified to behave like nativeQueryTest3? Or perhaps update this pull request. That is if you have an idea in mind of how to make the change.

[beikov]

Note that I can't add a challenge label to the issue

[scottmarlow]

Does anyone plan on updating this pr to update nativeQueryTest1 as mentioned above to address the challenge issue jakartaee/persistence#350?

I expect that jakartaee/persistence#350 will be marked accepted/invalid on Friday March 4th, so that we should be able to take action on this pull request at that time or exclude the challenged nativeQueryTest1 test.

[beikov]

I can do that, I just need to know whether we want to adapt the test so that it works like nativeQueryTest3 (which would make it useless I guess?), or if we want to change the test to assert an exception should be thrown.
I had the feeling that @lukasj should respond to my last comment on jakartaee/persistence#350 before I start working on this, so that we all understand why that test is wrong.

[beikov]

As discussed in jakartaee/persistence#350 we will remove the nativeQueryTest1