Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

323 custom rest headers#967

Closed
maltzj wants to merge 13 commits into
androidannotations:developandroidannotations/androidannotations:developfrom
maltzj:323_custom_rest_headersmaltzj/androidannotations:323_custom_rest_headersCopy head branch name to clipboard
Closed

323 custom rest headers#967
maltzj wants to merge 13 commits into
androidannotations:developandroidannotations/androidannotations:developfrom
maltzj:323_custom_rest_headersmaltzj/androidannotations:323_custom_rest_headersCopy head branch name to clipboard

Conversation

@maltzj

@maltzj maltzj commented Apr 17, 2014

Copy link
Copy Markdown
Contributor

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.

@WonderCsabo

Copy link
Copy Markdown
Member

@DayS @yDelouis, i do not use our REST API, but this PR seems to be useful. It should be reviewed.

@yDelouis

Copy link
Copy Markdown
Contributor

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.

@WonderCsabo

Copy link
Copy Markdown
Member

@maltzj can you refactor your PR as @yDelouis suggested? Without those changes we cannot merge it. :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use GitHub comments instead of commenting this kind of things directly into the code...

@WonderCsabo

Copy link
Copy Markdown
Member

@yDelouis What do you mean by becoming messy? I do not know the REST part of AA, but for me we have different handlers.

@yDelouis

Copy link
Copy Markdown
Contributor

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.

@WonderCsabo

Copy link
Copy Markdown
Member

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.

@WonderCsabo

Copy link
Copy Markdown
Member

@yDelouis can you refactor this PR?

@yDelouis

Copy link
Copy Markdown
Contributor

After 4.0.

@WonderCsabo

Copy link
Copy Markdown
Member

Refactored to #1470.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.