Diagnostics for invalid annotation parameter values

[rzgry]

Several mp fault tolerance annotations will throw a FaultToleranceDefinitionException if configured incorrectly. While I noticed this for fault tolerance, probably would be good to design this in a way that could be re-used for other mp-specs annotations.

A common scenario is parameters that have a min/max value

Ex. circuit breaker failure ratio must be between 0-1 https://github.com/eclipse/microprofile-fault-tolerance/blob/master/api/src/main/java/org/eclipse/microprofile/faulttolerance/CircuitBreaker.java#L149

or requestVolumeThreshold must be >= 1 https://github.com/eclipse/microprofile-fault-tolerance/blob/master/api/src/main/java/org/eclipse/microprofile/faulttolerance/CircuitBreaker.java#L136

Can look at the invalid parameters tests for the different cases annotations are invalid https://github.com/eclipse/microprofile-fault-tolerance/tree/5adfe495f0ad1fe8f52a443781b7509b01a92f15/tck/src/main/java/org/eclipse/microprofile/fault/tolerance/tck/invalidParameters

[datho7561]

Here is some info to help with implementing this issue:

Here is the current class we use to validate MicroProfile Fault Tolerance annotations: https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/internal/faulttolerance/java/MicroProfileFaultToleranceDiagnosticsParticipant.java

Here is a utils library that we have for helping work with annotations in JDT: https://github.com/eclipse/lsp4mp/blob/master/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/src/main/java/org/eclipse/lsp4mp/jdt/core/utils/AnnotationUtils.java

[rgrunber]

I'm generally ok, with adding the annotation support, but where I'd like to have some automation or generalization is in the validation of the parameters themselves. It seems overkill to have to hard-code specific parameter restrictions. Granted, we've done pretty much the same for #90 & likely for redhat-developer/quarkus-ls#378

I wonder if we could have some general definition file that specifies a format like :

@CircuitBreaker,failureRatio,double,0-1
@CircuitBreaker,successThreshold,int,>=1
...
...

or something similar that allows us to easily implement annotation validation for very basic parameter restrictions.

[angelozerr]

I agree with you, we should give the capability to describes validation rules in a metadata file. I think JSON format could be nice because we use already JSON for static properties.

I will work on that as soon as I will finish my tasks.

[angelozerr]

Here a proposition for describing the validation rules in a JSON file format:

[
	{
		"annotation": "org.eclipse.microprofile.faulttolerance.CircuitBreaker",
		"rules": [
			{
				"attribute": "delay",
				"ranges": ">=0"
			},
			{
				"attribute": "requestVolumeThreshold",
				"ranges": ">=1"
			},
			{
				"attribute": "failureRatio",
				"ranges": ">=0 and <=1"
			},
			{
				"attribute": "successThreshold",
				"ranges": ">=1"
			}			
		]		
	}
]

For ranges it requires to develop a parser to parse expression (by supporting basic syntax), but I find

{
  "attribute": "failureRatio",
  "ranges": ">=0 and <=1"
}

it's more readable than:

{
  "attribute": "failureRatio",
  "ranges": {
    "from" : {
      "value": "0",
      "inclusive": true
     },
     "to" : {
      "value": "1",
      "inclusive": true
     }
  }
}

The second advantage to use expression is that we could manage another expression in the future (if we need) like or instead of and.

@fbricon @rgrunber @AlexXuChen what do you think about that?

[angelozerr]

For loading the rules file /annotation-validators/mp-faulttolerance-validator.json, my idea is to declare it as an extension point in a plugin.xml like this:

   <extension point="org.eclipse.lsp4mp.jdt.core.annotationRules">
      <!-- Annotation rules for MicroProfile Fault Tolerance annotations -->
      <rule path="/annotation-validators/mp-faulttolerance-validator.json" />
   </extension>

[angelozerr]

After some experimentation, I think I will support this syntax:

{
  "attribute": "failureRatio",
  "ranges": {
    "from" : ">=0",
     "to" : "<=1"
  }
}

If you have some suggestion, please tell me. Thanks!

[angelozerr]

By continuing to develop this issue, I noticed that we visit the full AST for each diagnostic participant which I think which could be bad when we will add another diagnostic participant which will visist the AST.

My idea is to have one ASTVisitor which visist the full AST and this visitor should manage the whole collect of diagnostics. I'm working on this idea and my idea isto provide the extension point org.eclipse.lsp4mp.jdt.core.javaASTValidators instead of org.eclipse.lsp4mp.jdt.core.annotationRules. this extension point provide the capability to declare a validator which uses AST:

• it will wait for a class attribute (ex: validate the @ConfigProperty annaotation like we did)

   <extension point="org.eclipse.lsp4mp.jdt.core.javaASTValidators">
      <!-- Java validation for the MicroProfile @ConfigProperty annotation -->
      <validator annotation="org.eclipse.microprofile.config.inject.ConfigProperty" 
                 class="org.eclipse.lsp4mp.jdt.internal.config.java.MicroProfileConfigASTValidator" />
   </extension>

• or some parameter rules (to avoid declare those rules in a JSON file format like I suggested):

<extension point="org.eclipse.lsp4mp.jdt.core.javaASTValidators">
      <!-- Java validation for the MicroProfile Fault Tolerance annotations -->
      <validator annotation="org.eclipse.microprofile.faulttolerance.CircuitBreaker" >
         <parameter name="delay">
            <range from="&lt;=0" />
         </parameter>
         <parameter name="requestVolumeThreshold">
            <range from="&lt;=1" />
         </parameter>
         <parameter name="failureRatio">
            <range from="&lt;=0" to="&gt;=1" />
         </parameter>
         <parameter name="successThreshold">
            <range from="&lt;1" />
         </parameter>
      </validator>
   </extension>

[fbricon]

Sounds good. But I feel a bit cagey about using <=> symbols in the constraints. WDYT about using, for 0<x<=10,

<from excludeValue='true'>0<from>
<to>10</to>

, for >=1

<greaterThan>1</greaterThan>

...?

[angelozerr]

Sounds good.

Glad it pleases you.

But I feel a bit cagey about using <=> symbols in the constraints.

I think it's very verbose compare to just writting:

<range from="&lt;=0" to="&gt;=1" />

I find it's more natural to write those expression. @rgrunber what do you think?

[fbricon]

there's nothing natural in writing escaped XML (<)

[rgrunber]

I think from implies > or>=, and same for to, so I would remove the inequalities. Maybe you'd need an inclusive property to specify whether the = is there.

Alternative range syntax which I have always liked is from OSGi. eg. [1,10) for >= 1 && < 10

[angelozerr]

Indeed escape is awfull.

I like osgi syntax. @fbricon do you like it?

[fbricon]

+1

[angelozerr]

Ok now I'm using OSGI syntax which simplifies dragsticly the write of range:

   <extension point="org.eclipse.lsp4mp.jdt.core.javaASTValidators">
      <!-- Java validation for the MicroProfile Fault Tolerance annotations -->
      <annotationValidator annotation="org.eclipse.microprofile.faulttolerance.CircuitBreaker" >
         <parameter name="delay" range="0" /> <!-- x >=0 -->
         <parameter name="requestVolumeThreshold" range="1" /> <!-- x >=1 -->
         <parameter name="failureRatio" range="[0,1]" /> <!-- 0 <= x <= 1 -->
         <parameter name="successThreshold" range="1" /> <!-- x >=1 -->         
      </annotationValidator>
   </extension>

see

https://github.com/eclipse/lsp4mp/pull/173/files#diff-7281a15fa555693204939b8aaf950dd76009741a66ad70a8dccbe9f7627eb714R120

Many tahnks @rgrunber for your great suggestion!

[angelozerr]

I reopen this issue since I have just supported CircuitBreak rules.

@AlexXuChen could you see other cases in https://github.com/eclipse/microprofile-fault-tolerance/tree/5adfe495f0ad1fe8f52a443781b7509b01a92f15/tck/src/main/java/org/eclipse/microprofile/fault/tolerance/tck/invalidParameters

to support that:

• you must write rules like

  lsp4mp/microprofile.jdt/org.eclipse.lsp4mp.jdt.core/plugin.xml

  Line 130 in 8531fee

  <annotationValidator annotation="org.eclipse.microprofile.faulttolerance.CircuitBreaker"

• write tests like

  lsp4mp/microprofile.jdt/org.eclipse.lsp4mp.jdt.test/src/main/java/org/eclipse/lsp4mp/jdt/core/faulttolerance/java/MicroProfileFaultToleranceJavaDiagnosticsTest.java

  Line 131 in 8531fee

  public void circuitBreakerClientForValidationDelay() throws Exception {

[AlexXuChen]

I believe we can close this issue @angelozerr. Thoughts?

[angelozerr]

yes sure, since your Retry PR has been merged.