-
Notifications
You must be signed in to change notification settings - Fork 472
Add extra attributes to Link and use for HAL mediatype #567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9d96a11
to
2dfa996
Compare
@olivergierke I redid this PR so that it doesn't attempt to clean up any whitespace. |
2dfa996
to
520122f
Compare
520122f
to
57c90e4
Compare
Hi @gregturn @olivergierke |
@netwolfuk I can certainly comment that Oliver was out for a couple weeks, so he has been catching up on a stack of stuff across the Spring Data suite, the biggest HATEOAS one being the Affordances API effort. The goal is to at least have that pulled in. I'm working to get several other patches pulled in as well (may be at least 7 other PRs I have yet to review with Oliver). What may be highly pertinent to this PR is that the Affordances PR ALSO pulls in extra RFC 5988 material, so I'll have to certainly reevaluate this one when the dust settles. As to how many fit into 0.24, that has yet to be determined. It may be shaped by the state of Spring Data's release train when we finish Affordances. Sorry about being so vague. Wanted to give you insight into the process. |
@gregturn Thanks for the thorough response. I really appreciate all you guys do. |
Would highly appreciate if this would be merged and released with the next version, as it would be very useful to us. Thanks for implementing @gregturn |
I'm shooting to get all the RFC 5988 material included. Just want a coherent solution between this PR and Affordances. |
* @param type | ||
* @param deprecation | ||
*/ | ||
public Link(String href, String rel, String hreflang, String media, String title, String type, String deprecation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to use Lombok here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* | ||
* @return | ||
*/ | ||
public String getHreflang() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lombok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* @param hreflang | ||
* @return | ||
*/ | ||
public Link withHreflang(String hreflang) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lombok's @Wither
? Should we have withHref(…)
and withRel(…)
, too, for consistency reasons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -212,7 +347,20 @@ public boolean equals(Object obj) { | |||
|
|||
Link that = (Link) obj; | |||
|
|||
return this.href.equals(that.href) && this.rel.equals(that.rel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EqualsAndHashCode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -263,7 +448,34 @@ public static Link valueOf(String element) { | |||
throw new IllegalArgumentException("Link does not provide a rel attribute!"); | |||
} | |||
|
|||
return new Link(matcher.group(1), attributes.get("rel")); | |||
Set<String> unrecognizedHeaders = unrecognizedHeaders(attributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following Postel's Law I think we should not throw an exception here but just ignore attributes that we don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Field annotations inline. Formatting.
That's merged. |
@olivergierke @gregturn I am facing the similar issue... I would like to add an extra attribute along with "href". May I know how to do it? Any help would be appreciable. I am using spring-boot 1.5.8 along with Hateoas |
Not sure what attribute you seek. What mediatype can this property be found in? To answer your question, We don't lots of requests of people needed to customize the |
Adds many additional attributes defined in RFC5988 and verifies they work properly in the neutral representation of Link while also being rendered properly in the HAL module.
Related tickets: #100, #417, #235
Previous pull requests: #240, #238, #223, #79