search

jakartaee / rest

Jakarta RESTful Web Services
Other
353 stars 114 forks source link

TCK Challenge: Change JsonbContextProviderIT.EchoApplication to be public #1099

Closed jamezp closed 2 years ago

jamezp commented 2 years ago

I filed a PR https://github.com/jakartaee/rest/pull/1097 to make the JsonbContextProviderIT.EchoApplication public rather than private. This was given a -1 from @mkarg, but I'd like to challenge this.

RESTEasy has required constructors for applications be public for at least 11 years, according the git history. I don't see any reason why this should change, especially for a TCK test. If RESTEasy starts allowing non-public constructors we now have to support this going forward and I'm opposed to this.

jansupol commented 2 years ago

There would be more of them. I already commented about this.

jamezp commented 2 years ago

Do we know if Jersey allows non-public constructors for applications? All other REST resources seem to require public constructors. Having one type be different seems wrong to me regardless of whether the spec explicitly does not state it. It also doesn't state it can be non-public.

jansupol commented 2 years ago

Surprisingly, in Jersey, the tests pass fine. But I am +1 for updating the Application subclasses visibility, we may get hit by this when converging towards CDI, too. The tests are meant to test other functionality than private class instantiation. That could have been done in other tests if required.

jamezp commented 2 years ago

Good to know, thank you. The last CI job I saw showed it failing, but for a completely different reason.

mkarg commented 2 years ago

The problem is that what you like to change is a spec change. So feel free to propose a spec change. The TCK test is compliant with the spec hence I am -1 for changing this. It is not a TCK challenge.

arjantijms commented 2 years ago

+1 for the change in #1097.

Adding a test with something that was not tested before and for which the spec document doesn't explicitly say something is effectively a spec change. Instead of just adding a test to test that private applications are possible, I think this should have been discussed first.

Then only after everybody agreed that private applications should be possible, can the test be added (and then the spec document should be clarified).

spericas commented 2 years ago

I'm +1 for #1097. There's no need to make this more difficult to instantiate unless that is the assertion being tested by the TCK, which is not. Any further clarification may come later, now we need to complete 3.1, so we can leave the discussion for later and accept the PR now.

scottmarlow commented 2 years ago

Could a committer please add challenge label for this issue.

mkarg commented 2 years ago

+1 for the change in #1097.

Adding a test with something that was not tested before and for which the spec document doesn't explicitly say something is effectively a spec change. Instead of just adding a test to test that private applications are possible, I think this should have been discussed first.

Then only after everybody agreed that private applications should be possible, can the test be added (and then the spec document should be clarified).

Sorry but this is complete nonsense. The spec is the sole source of truth. Any application developer may do anything he likes unless forbidden by the spec. That is why it is explicitly forbidden for resources. Any restriction we give upon normal Java must be explicitly told in the spec. Until it is, the TCK's recent behavior was just incomplete and wrong, as not the TCK but the spec is the sole truth.

jamezp commented 2 years ago

@mkarg Would you accept a test which which includes an implementation of an Application which accepts a parameter because the specification doesn't specify it?

arjantijms commented 2 years ago

Sorry but this is complete nonsense.

It's not nonsense IMHO. I think it makes a lot of sense really.

Any application developer may do anything he likes unless forbidden by the spec.

I don't think that's how the spec works. We test its assertions, which are explicit statements in the spec. We don't test the opposite, which is the absence of some statement.

The only thing that can happen is that a tests just happens to use something while testing something else, and it's not discovered. I think that's what's going on here. Maybe this test just happened (by accident) to make the class private. Luckily this was discovered, hence this challenge here.

Clearly the spec is silent about these private classes, so it would need clarification. These (small) things where the spec was silent about something essentially took me the last 10 years to get clarified in the various security specs. Every time we did the same thing; first get clarification, then add a test asserting this.

spericas commented 2 years ago

I don't think that's how the spec works. We test its assertions, which are explicit statements in the spec. We don't test the opposite, which is the absence of some statement.

The only thing that can happen is that a tests just happens to use something while testing something else, and it's not discovered. I think that's what's going on here. Maybe this test just happened (by accident) to make the class private. Luckily this was discovered, hence this challenge here.

I don't know how to explain this more clearly. The TCK test needs updating.

scottmarlow commented 2 years ago

Some guidance from the TCK Process 1.2 under "Valid Challenges":

The following scenarios are considered in scope for test challenges:

  1. Claims that a test assertion conflicts with the specification.
  2. Claims that a test asserts requirements over and above that of the specification.
  3. Claims that an assertion of the specification is not sufficiently implementable.
  4. Claims that a test is not portable or depends on a particular implementation.
mkarg commented 2 years ago

(a) Which of the above cases is the one here?

(b) Why don't we simply change the spec, as apparently we agree that it should?

mkarg commented 2 years ago

@mkarg Would you accept a test which which includes an implementation of an Application which accepts a parameter because the specification doesn't specify it?

Yes I would, because the spec does not forbid it.

Anyways, looking at CDI, we should just change the spec to enforce public.

jamezp commented 2 years ago

(a) Which of the above cases is the one here?

I would say 2 and 4 personally.

(b) Why don't we simply change the spec, as apparently we agree that it should?

That seems fine to me as well. I'm not really sure what the best wording would be as it seems things may change in 4.0 if CDI is going become more into play here.

spericas commented 2 years ago

Updating the spec is not ideal (and I really don't like to do this at the last moment) given that the release process is already underway and only awaiting the TCK's. Updating the single TCK test is trivial and inline with the guidelines outlined above. Once again, the proposed test change does not correlate with any spec assertion.