Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Section 11.5.2 refers to an unknown class #295

Closed
snicoll opened this issue Feb 17, 2014 · 6 comments
Closed

Section 11.5.2 refers to an unknown class #295

snicoll opened this issue Feb 17, 2014 · 6 comments
Labels
Milestone

Comments

@snicoll
Copy link

snicoll commented Feb 17, 2014

Section 11.5.2 defines what happens when more than one annotations are placed on the same method. In this case a CacheAnnotationConfigurationException should be thrown but this exception is not available anywhere.

@gregrluck
Copy link
Member

This was unused in the code base so I removed it. Thanks for picking this up. Will bring this back in the first maintenance release.

@gregrluck gregrluck added this to the 1.1 milestone Mar 26, 2014
@gregrluck gregrluck self-assigned this Oct 10, 2015
@snicoll
Copy link
Author

snicoll commented Mar 28, 2017

@gregrluck I was discussing this one with @jhoeller and we're wondering why this is part of the spec at all. If you've misused the annotation model, what does a standard exception bring concretely?

We'd rather remove that section or rephrase it so that an exception must be thrown but not specifying a concrete exception type.

@cruftex
Copy link
Member

cruftex commented Mar 29, 2017

We'd rather remove that section or rephrase it so that an exception must be thrown but not specifying a concrete exception type.

Answering from a maintenance point of view:

Rewording from "should" to "must" calls for proper TCK tests. That can break existing implementations and causes work for an ambiguous situation, but that has no relevance for working application code.
Further more, it is too late to start discussion about consistent handling of configuration errors.

If the "should" stays, then at least the RI should have the behavior that we endorse in the spec.

Considering this, I am in favor for:

  • Don't include the exception for 1.1, it was not there in 1.0 and nobody says it's needed
  • Reword spec: Ambiguous configurations should cause implementation defined exceptions.

@snicoll : Since you are the initial and only requester, what is your preferred resolution?

@cruftex cruftex reopened this Mar 29, 2017
@snicoll
Copy link
Author

snicoll commented Mar 31, 2017

Don't include the exception for 1.1, it was not there in 1.0 and nobody says it's needed

The spec does but I see what you mean. Just to make sure I understand, your option is to do both of the items in the bullet list, right? That's what I'd do: remove the exception and any reference in the spec and reword it to states that implementations must throw defined exceptions.

Thanks!

@cruftex
Copy link
Member

cruftex commented Apr 1, 2017

your option is to do both of the items in the bullet list, right?

Yes!

... reword it to states that implementations must throw defined exceptions.

Careful. That formulation is to strict. It says that an implementation must throw an exception but maybe it has enough information to continue and only log a warning. Personally, I like the "fail fast and hard" approach but implementations can have different preference. Strict formulations also require tests. I am going with RFC-like wording: normative/strict => must, recommendation => should.

Let's see whether somebody else has another opinion on it, otherwise I think we can go forward.

@cruftex
Copy link
Member

cruftex commented Oct 7, 2017

On page 121 the standard says:

11.5.2. Multiple Annotations

Only one method level caching annotation can be specified on a method and only one parameter level
caching annotation can be specified on a parameter. If more than one annotation is specified on a
method or on a parameter then a CacheAnnotationConfigurationException must be thrown
either at application initialization time or on method invocation.

Proposed change:

  • revert commit e7b9606
  • Change the last sentence to: If more than one annotation is specified on a method, annotation processors are recommended to throw an implementation specific exception or log a warning.

Rationale:

  • No annotation processor is implementing this, since the exception is missing in 1.0
  • It is not likely that implementations will change for 1.1 and comply to the standard
  • We had the discussion about configuration errors at other points as well and decided not to enforce additional behavior in an MR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants