Skip to content
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 from
Closed

323 custom rest headers #967

maltzj wants to merge 13 commits into from

Conversation

@maltzj
Copy link
Contributor

@maltzj maltzj commented Apr 17, 2014

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
Member

WonderCsabo commented Jun 29, 2014

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

@yDelouis
Copy link
Contributor

yDelouis commented Jun 29, 2014

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
Member

WonderCsabo commented Aug 22, 2014

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

WonderCsabo
WonderCsabo reviewed Aug 22, 2014
View reviewed changes
...ons/androidannotations/src/main/java/org/androidannotations/helper/RestAnnotationHelper.java
Copy link
Member

@WonderCsabo WonderCsabo Aug 22, 2014

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
Member

WonderCsabo commented Aug 22, 2014

@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
Contributor

yDelouis commented Aug 22, 2014

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
Member

WonderCsabo commented Aug 22, 2014

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
Member

WonderCsabo commented Mar 22, 2015

@yDelouis can you refactor this PR?

@yDelouis
Copy link
Contributor

yDelouis commented Mar 23, 2015

After 4.0.

@WonderCsabo WonderCsabo mentioned this pull request Jun 5, 2015
Merged
@WonderCsabo WonderCsabo assigned WonderCsabo and unassigned yDelouis Jun 16, 2015
@WonderCsabo WonderCsabo mentioned this pull request Jun 16, 2015
Merged
@WonderCsabo
Copy link
Member

WonderCsabo commented Jun 16, 2015

Refactored to #1470.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maltzj @WonderCsabo @yDelouis