Add HAL JSON UTF-8 variant to MediaTypes #470

drumonii · 2016-07-26T03:14:01Z

Add ;charset=UTF-8 to application/hal+json similar to APPLICATION_JSON_UTF8 in MediaType

eneveu · 2016-11-10T10:15:13Z

src/main/java/org/springframework/hateoas/MediaTypes.java

+	/**
+	 * A String equivalent of {@link MediaTypes#HAL_JSON_UTF8}.
+	 */
+	public static final String HAL_JSON_UTF8_VALUE = HAL_JSON_VALUE + ";charset=UTF-8";


Instead of doing String concatenation, I would write = "application/hal+json;charset=UTF-8", to make this easier to grep and to understand. It would be similar to org.springframework.http.MediaType.

Also, I think we should add an underscore to "UTF8" in the constant names: HAL_JSON_UTF_8_VALUE. It's more readable, and it seems to be more conventional when looking at the Spring / Guava / JDK codebases.

I agree about the String concatenation, but it's interesting you mention MediaType because that's where I modeled my hal+json additions from. See this line

Also it uses UTF8 instead of UTF_8 as you suggested. Just want to be consistent how the current framework does it.

Indeed, I missed that "APPLICATION_JSON_UTF8_VALUE" constant in MediaType... It's weird that they chose to write it like that, since Guava / Spring / JDK seem to prefer the underscore everywhere else (I even cloned a few Spring project and grep'ed to check :D ).

I guess it makes sense in a way, since you clearly separate the "HAL" / "JSON" and "UTF 8" info. We should probably go for consistency with MediaTypes.

Anyway I created a temporary class "AdditionalMediaTypes" in my project, until we can get this in Spring HATEOAS. Great suggestion :)

Thanks! Yeah, I had to do the same to have a hal+json media type with ;charset=UTF-8. Hopefully this gets in the next release.

I agree it's an odd way to write it but in this situation I'd rather keep it as Spring's MediaType does rather than be different only here.

Based upon Spring's MediaType being:

/** * Public constant media type for {@code application/json;charset=UTF-8}. */ public final static MediaType APPLICATION_JSON_UTF8; /** * A String equivalent of {@link MediaType#APPLICATION_JSON_UTF8}. */ public final static String APPLICATION_JSON_UTF8_VALUE = APPLICATION_JSON_VALUE + ";charset=UTF-8";

...I think the proposed fix is crafted properly.

marceloverdijk · 2017-03-06T07:52:14Z

This would be a nice addition especially as a default Boot app with the hateoas starter adds the charset to the content type.

SympathyForTheDev · 2017-03-20T16:53:45Z

+1 for this addition

gregturn · 2017-03-20T21:32:02Z

It appears that the Spring Framework has APPLICATION_JSON_UTF8 only to support the fact that MappingJackson2HttpMessageConverter defaults to application/json ;charset=UTF-8. In other words, in that same class, there are no other UTF-8 types added to the vast list of IANA spec'd mediatypes.

So my question is, why do we need to add one for HAL in this project? Is something breaking because of it? Can you draft a test case or scenario where this is causing issues? Otherwise, most of the dialog has been about how to implement the solution to a problem not well defined.

drumonii · 2017-03-21T00:12:29Z

Spring Boot defaults its HTTP encoding to UTF-8 since 1.2, see here.

The issue that I came across is that if you had a test like this, it would fail.

mockMvc.perform(get("/api/test"))
		...
		.andExpect(content().contentType(MediaTypes.HAL_JSON))

java.lang.AssertionError: Content type 
Expected :application/hal+json
Actual   :application/hal+json;charset=UTF-8

Since there is a UTF-8 version of application/json in Spring Framework's MediaType, thought it would be useful if SDR had the UTF-8 version of application/hal+json in its MediaTypes for consistency.

gregturn · 2017-03-26T17:48:04Z

Additionally, we need some tests for this mediatype such as in EnableHypermediaSupportIntegrationTest and TypeReferencesIntegrationTest.

In fact, if you blanket check for usage of the media type, there are other classes that must be revised based on such a change including: HypermediaSupportBeanDefinitionRegistrar and HalLinkDiscovered.

Updating these spots in this PR would go a long way toward polishing things up for merging.

drumonii · 2017-03-27T16:49:42Z

@gregturn I'll take a look at updating the tests. Will get back to you in Spring Data Gitter if I have questions.

gregturn · 2017-03-27T18:42:18Z

Thanks. FYI @'ing me here is more effective to reach me than gitter. :)

drumonii · 2017-03-28T13:41:50Z

src/main/java/org/springframework/hateoas/core/JsonPathLinkDiscoverer.java

 	 */
-	public JsonPathLinkDiscoverer(String pathTemplate, MediaType mediaType) {
-
+	public JsonPathLinkDiscoverer(String pathTemplate, MediaType... mediaTypes) {


Wasn't quite sure what to do with this constructor. It seems that the MediaType is nullable so I made it varargs. I thought about another constructor with a List of MediaType...

drumonii · 2017-03-28T13:42:12Z

src/main/java/org/springframework/hateoas/core/JsonPathLinkDiscoverer.java

+		if (this.mediaTypes == null) {
+			return true;
+		} else {
+			for (MediaType mediaType : this.mediaTypes) {


Wasn't sure what to do here either with the List of MediaType and isCompatibleWith

drumonii · 2017-03-28T13:43:20Z

src/test/java/org/springframework/hateoas/mvc/TypeReferencesIntegrationTest.java

@@ -82,12 +83,17 @@ public void setUp() {
 	@Test
 	public void usesResourceTypeReference() {

-		server.expect(requestTo("/resource")).andRespond(withSuccess(RESOURCE, MediaTypes.HAL_JSON));
+		server.expect(times(1), requestTo("/resource")).andRespond(withSuccess(RESOURCE, MediaTypes.HAL_JSON));
+		server.expect(times(1), requestTo("/resource")).andRespond(withSuccess(RESOURCE, MediaTypes.HAL_JSON_UTF8));


Duplicated server expect with MediaTypes.HAL_JSON_UTF8. Not sure the preferred way to test the MediaTypes.HAL_JSON in addition to MediaTypes.HAL_JSON_UTF8

drumonii · 2017-03-28T13:44:42Z

@gregturn initial polish of PR with tests is up! See my comments on unsure spots. Let me know what changes or additions elsewhere should be made. Thanks.

gregturn · 2017-04-05T22:04:36Z

@drumonii With all the work in Berlin this week, I can hopefully take a close look next week.

drumonii · 2017-04-05T22:20:06Z

@gregturn No worries, that sounds good!

SympathyForTheDev · 2017-07-19T12:32:50Z

Hello, In what version of hateos this fix will be available ?

gregturn · 2017-07-19T12:39:44Z

I honestly don't know. Outcome of Berlin was to work on Affordances. That took weeks and based on the outcome we are taking a different approach. I'm presently coding HAL-Forms media type and using all my lessons learned on Affordances to write a lighter version of such an API. That along with all the work on Spring Data 2 has barred Oliver from reviewing the other PRs on this project.

odrotbohm · 2017-09-07T11:21:53Z

@drumonii — Would you mind to provide us with your full name and a proper email address?

odrotbohm · 2017-09-07T11:31:27Z

Actually, nevermind. I'll have to roll back most of the changes you made anyway except the addition of the addition of the media types.

@Drummond

Added application/hal+json;charset=UTF-8 to the media types enum. Traverson now configures all HAL flavors to be supported by the registered HAL-specific HttpMessageConverter. Removed nullability of media type in JsonPathLinkDiscoverer. This requires subclasses that want to support all media types to explicitly configure MediaType.ALL. Heavily inspired by the PR submitted by @Drummond but very significantly rewritten.

eneveu reviewed Nov 10, 2016

View reviewed changes

gregturn added the type: enhancement label Mar 20, 2017

gregturn added the process: waiting for feedback label Mar 20, 2017

odrotbohm force-pushed the master branch from 0e8b0e1 to aafe1f2 Compare March 26, 2017 09:19

gregturn mentioned this pull request Mar 26, 2017

When using toResources(), "links" is not correctly marshalled #493

Closed

drumonii added 2 commits March 27, 2017 22:12

Merge remote-tracking branch 'refs/remotes/spring-projects/master'

579f1f8

Add HAL_JSON_UTF8 to MediaTypes

214646a

drumonii commented Mar 28, 2017

View reviewed changes

gregturn pushed a commit that referenced this pull request May 23, 2017

#470 - Add HAL_JSON_UTF8 to MediaTypes

ad39339

gregturn added process: in progress and removed process: waiting for feedback labels May 23, 2017

gregturn self-assigned this May 23, 2017

odrotbohm assigned odrotbohm and unassigned gregturn Sep 7, 2017

odrotbohm added this to the 0.24 milestone Sep 7, 2017

odrotbohm closed this Sep 7, 2017

odrotbohm changed the title ~~Add HAL_JSON_UTF8 to MediaTypes~~ Add HAL JSON UTF-8 variant to MediaTypes Sep 7, 2017

Add HAL JSON UTF-8 variant to MediaTypes #470

Add HAL JSON UTF-8 variant to MediaTypes #470

Uh oh!

Conversation

drumonii commented Jul 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marceloverdijk commented Mar 6, 2017

Uh oh!

SympathyForTheDev commented Mar 20, 2017

Uh oh!

gregturn commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drumonii commented Mar 21, 2017

Uh oh!

gregturn commented Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drumonii commented Mar 27, 2017

Uh oh!

gregturn commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drumonii commented Mar 28, 2017

Uh oh!

gregturn commented Apr 5, 2017

Uh oh!

drumonii commented Apr 5, 2017

Uh oh!

SympathyForTheDev commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregturn commented Jul 19, 2017

Uh oh!

odrotbohm commented Sep 7, 2017

Uh oh!

odrotbohm commented Sep 7, 2017

Uh oh!

Uh oh!

drumonii commented Jul 26, 2016 •

edited

Loading

gregturn commented Mar 20, 2017 •

edited

Loading

gregturn commented Mar 26, 2017 •

edited

Loading

SympathyForTheDev commented Jul 19, 2017 •

edited

Loading