You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It seemed like some of the stuff under the contributions page didn't apply to this PR (I don't think I should be deploying new snapshots willy nilly for example), so I may have done this wrong.
I also manually checked the code that the tests output, but I would feel better if there were automated testing in place to be sure that headers were properly set.
Lastly, I think this code will break if there is both a @RequiresHeader and a @Header annotation on the same method. Users would have to be pretty dumb to do this, but it is definitely a thing that is conceivable.
Let me know what you all need in the way of support for this.
I reviewed this PR quickly and I have some comments :
You should create a Handler for these two annotations : Header and Headers to be able to validate them. To be sure they are used on methods in an interface annotated with @rest.
You should add a some tests in functional-tests so that we can check whether the generated code is right.
Another comment, not related directly to this PR :
The REST part of AA is becoming very messy. An annotation should be handled by the corresponding handler and only by it Here we have one handler handling at least 6 annotations.
The method declareHttpHeaders in RestAnnotationHelper is 70 lines long for example and deals with @Accept, @Headers, @RequireCookie, @ RequiresAuthentication...
That makes annotations depend on each other which is never a good thing.
OK, i suspected that RestAnnotationHelper has something to do with it. If you think the problem is serious, open a new issue about refactoring the REST part.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It seemed like some of the stuff under the contributions page didn't apply to this PR (I don't think I should be deploying new snapshots willy nilly for example), so I may have done this wrong.
I also manually checked the code that the tests output, but I would feel better if there were automated testing in place to be sure that headers were properly set.
Lastly, I think this code will break if there is both a @RequiresHeader and a @Header annotation on the same method. Users would have to be pretty dumb to do this, but it is definitely a thing that is conceivable.
Let me know what you all need in the way of support for this.