workflowcheck - initial support for Java static analyzer #2356
Conversation
| } | ||
|
|
||
| // Need all-in-one JAR | ||
| shadowJar { |
There was a problem hiding this comment.
Unlike the -shaded project, we just need the -all.jar this generates, and we only have one real dependency anyways
| // Add the workflowcheck project as a composite build. We are only doing this | ||
| // for the sample, normally this is not needed. | ||
| includeBuild '../../../' No newline at end of file |
There was a problem hiding this comment.
This and the other two samples will be updated when a version of this project is published
| System.err.println("Unrecognized argument: " + invalidArg); | ||
| } | ||
|
|
||
| System.err.println("Analyzing classpath for classes with workflow methods..."); |
There was a problem hiding this comment.
Not sure why I do System.err here and System.out below, probably just need to pick one
|
|
||
| # Quite a few internal libraries catch interrupts just to re-interrupt, so we | ||
| # will mark thread interrupt as safe (other thread stuff is not) | ||
| temporal.workflowcheck.invalid.java/lang/Thread.currentThread=false |
There was a problem hiding this comment.
Can you check my understand of the format here, so above you said thread was not deterministic and here you are allowlisting these specific methods is that fair to say? So all other static method in Thread are not allowed.
There was a problem hiding this comment.
Yes, that is fair to say. Correct, other static methods in thread are not allowed (i.e. considered "invalid" and set to true) due to the higher level rule above. This works because of the order of checking from most specific to least specific until it finds a decision. This is admittedly a bit complicated, but I think it's a very sane/flexible approach. The "Configuration" section of the README goes over how it works.
So even if a user wanted to set one thread method allowed in their config on top of this, they can.
|
Note, we should remove the copyright notice from the top of the files |
|
There are a few |
| # limitations under the License. | ||
| # | ||
|
|
||
| #### Invalid Calls #### |
There was a problem hiding this comment.
One thing that would be nice to add is a suggested resolution for a violation, ie if the used called Clock we should suggest they use Workflow.currentMillis(). I don't think you need to add that right now, but can you suggest how we would add that in the future with this format?
There was a problem hiding this comment.
Hrmm, yeah I suppose in addition to false and true, we could accept any arbitrary string and that would be implied true and be the error message. Or even better, the value is prefixed with message: to future proof it and have more clear true/false/message: validation.
Can do. Want me to do that here in PR, or want to do that as part of some project move to another repo?
No, not part of initial release, they should be opened as separate issues IMO or decided not worth doing. And the entire "TODO" section of the README should be removed (and maybe the TODOs in code too, less concerned about those). I can open these, but I'm guessing we should confirm the home for this first. |
2 participants
What was changed
Added initial
temporal-workflowcheckproject. This is currently considered "beta". See the README for more about the project.TODO in the near future: