Skip to content

Conversation

@gregturn
Copy link
Contributor

@gregturn gregturn commented Dec 7, 2018

In HAL-based documents (HAL and HAL-FORMS), the "templated" property must ignored in LinkMixin, because that's where it is applied. Removed the need for a no-op setTemplated method in Link.

  • Test cases drafted for both HAL an HAL-FORMS to verify proper behavior.
  • Jackson2HalFormsIntegrationTest migrated from Hamcrest to AssertJ assertions.

Related pull requests: #668

In HAL-based documents (HAL and HAL-FORMS), the "templated" property must be ignored in LinkMixin, because that's where it is applied. Removes the need for a no-op `setTemplated` method in Link.

* Test cases drafted for both HAL an HAL-FORMS to verify proper behavior.
* Jackson2HalFormsIntegrationTest migrated from Hamcrest to AssertJ assertions.

Related pull requests: #668
Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

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

One thing to realize is that this change means that we are going to ignore quite some cases of misspelled attributes.

Not sure if we consider this a problem.
But I personally prefer the old version where we only ignore the templated argument.

@gregturn
Copy link
Contributor Author

gregturn commented Dec 10, 2018

@schauder Yes, this setting would allow MANY properties that are mistyped or wrong to slip through the cracks, but that is actually a Good Thing™.

For RESTful apps, you generally want to avoid fast failing over properties not recognized. That's because those might be properties added in future versions of the service that the current version of the service doesn't recognize.

When anyone uses Spring HATEOAS straight up, or through Spring Data REST, Spring HATEOAS's @EnableHypermediaSupport annotation actually activates a bean post processor that will find all instances of RestTemplate and register converters for hypermedia with these settings:

private MappingJackson2HttpMessageConverter createHalConverter(ObjectMapper objectMapper, CurieProvider curieProvider,
		RelProvider relProvider, MessageSourceAccessor linkRelationMessageSource) {

	HalConfiguration halConfiguration = this.halConfiguration.getIfAvailable(() -> new HalConfiguration());

	HalHandlerInstantiator instantiator = new Jackson2HalModule.HalHandlerInstantiator(relProvider, curieProvider,
			linkRelationMessageSource, halConfiguration);

	ObjectMapper mapper = objectMapper.copy();
	mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
	mapper.registerModule(new Jackson2HalModule());
	mapper.setHandlerInstantiator(instantiator);

	MappingJackson2HttpMessageConverter converter = new TypeConstrainedMappingJackson2HttpMessageConverter(
			ResourceSupport.class);
	converter.setSupportedMediaTypes(Arrays.asList(HAL_JSON, HAL_JSON_UTF8));
	converter.setObjectMapper(mapper);

	return converter;
}

(https://github.com/spring-projects/spring-hateoas/blob/master/src/main/java/org/springframework/hateoas/config/ConverterRegisteringWebMvcConfigurer.java#L178-L197)

That's a lot, but nestled in the middle of configuring HAL support, we deliberately disable Jackson's FAIL_ON_UNKNOWN_PROPERTIES option.

We even petitioned Jackson to flip their default setting on this property, but they said no.

The tradeoff, which I'm sure you're smelling, is that you are disabling fast failure. For many scenarios, we prefer to fail fast, but in RESTful backward compatibility, we do NOT.

It means that someone could ALMOST have the right property, but if they don't it won't get picked up. Spring Data has near-miss algorithms to detect if someone ALMOST has the right property name in query derivation, and will spit out a message helping the user. We do not.

If you're curious why the post processor code above isn't handling "templated" property names, it's because Jackson mixins override POJO Jackson settings. A corner case mended with this PR.

@gregturn gregturn closed this Dec 10, 2018
@gregturn gregturn deleted the bug/templated-hal branch December 10, 2018 14:22
@gregturn
Copy link
Contributor Author

Resolved via 8bf01c3.

@gregturn gregturn self-assigned this Dec 10, 2018
@gregturn gregturn added this to the 1.0 M1 milestone Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants