Provide a public API for resolving the type of a field #549

mduesterhoeft · 2018-09-05T20:18:01Z

This can be useful for extensions to reuse field type determination logic.

mduesterhoeft · 2018-09-06T06:52:06Z

@wilkinsona this is what we talked about in the chat the other day. Is that going into the right direction?

🤔 The Codacy complaint is not valid I think - and I just moved the code around. But if you want me to change the enum comparison from == to equals I would be happy to do this.

wilkinsona · 2018-09-06T08:29:17Z

@mduesterhoeft Thanks for the PR and sorry for the annoyance with Codacy. It's been irritating me for a little while now. Your comment has given me the nudge that I needed to remove the integration.

mduesterhoeft · 2018-09-06T11:46:44Z

@wilkinsona no worries - I always struggle with myself if such code analysis is useful or not. I guess sometimes it is - sometimes it is rather annoying.

wilkinsona

Thanks again the PR. On the whole, things look good. I left a handful of relatively minor comments. Could you take a look at those when you have a moment please?

wilkinsona · 2018-10-03T17:02:46Z

...tdocs-core/src/main/java/org/springframework/restdocs/payload/ContentTypeHandlerFactory.java

@@ -0,0 +1,67 @@
+/*
+ * Copyright 2014-2016 the original author or authors.


This should be 2014-2018.

wilkinsona · 2018-10-03T17:04:22Z

spring-restdocs-core/src/main/java/org/springframework/restdocs/payload/FieldTypeResolver.java

+ * payloads.
+ *
+ * @author Mathias Düsterhöft
+ */


This is new public API so it should be annotated with @since. I think I'd like to sneak this into 2.0.x, so it should be @since 2.0.3.

wilkinsona · 2018-10-03T17:08:21Z

spring-restdocs-core/src/main/java/org/springframework/restdocs/payload/FieldTypeResolver.java

+	 * @param contentType the content type of the payload
+	 * @return the {@link FieldTypeResolver}
+	 */
+	static FieldTypeResolver create(byte[] content, MediaType contentType) {


I think a name like forContent may read a little better. What do you think?

wilkinsona · 2018-10-03T17:11:17Z

...restdocs-core/src/test/java/org/springframework/restdocs/payload/FieldTypeResolverTests.java

+
+	@Test
+	public void returnJsonFieldTypeResolver() {
+		assertThat(FieldTypeResolver.create("{\"field\": \"value\"}".getBytes(),


Could you use ExpectedException here please? I'd like to move to AssertJ's assertThatExceptionOfType, but until that change is made across the whole codebase I'd prefer to make consistent use of ExceptedException.

wilkinsona

Thanks again. Sorry, but in getting ready to merge this, I noticed that things aren't quite as close as I'd initially thought. I've left a few more comments. Could you take a look again when you have a moment please?

wilkinsona · 2018-10-04T12:11:07Z

...tdocs-core/src/main/java/org/springframework/restdocs/payload/ContentTypeHandlerFactory.java

+ *
+ * @author Mathias Düsterhöft
+ */
+final class ContentTypeHandlerFactory {


I think this could be replaced with a static forContent method on ContentHandler.

wilkinsona · 2018-10-04T12:11:41Z

spring-restdocs-core/src/main/java/org/springframework/restdocs/payload/FieldTypeResolver.java

+	 * @param fieldDescriptor the field descriptor
+	 * @return the type of the field
+	 */
+	Object determineFieldType(FieldDescriptor fieldDescriptor);


For consistency with the other Resolver classes in REST Docs, this method should be named resolveFieldType.

wilkinsona · 2018-10-04T12:17:00Z

spring-restdocs-core/src/main/java/org/springframework/restdocs/payload/JsonContentHandler.java

@@ -144,30 +147,4 @@ private boolean isEmpty(Object object) {
 		return ((List<?>) object).isEmpty();
 	}

-	@Override
-	public Object determineFieldType(FieldDescriptor fieldDescriptor) {


I think this logic should remain here. It's subtle, but there's a difference between resolving the type of a field (which is what the field type resolver does) and determining the type of a field. It's only the latter that considers the type that's specified on the descriptor and checks whether or not they match. Ideally the former would only use the path (it currently looks at whether the field is optional too). I'd like to keep the current level of separation.

wilkinsona · 2018-10-04T12:18:19Z

...estdocs-core/src/test/java/org/springframework/restdocs/payload/JsonContentHandlerTests.java

 */
 public class JsonContentHandlerTests {

 	@Rule
 	public ExpectedException thrown = ExpectedException.none();

-	@Test
-	public void typeForFieldWithNullValueMustMatch() {


Given my comment above, I think these tests should remain here.

…solver.

mduesterhoeft · 2018-10-04T20:12:22Z

@wilkinsona I refactored to be able to keep the separation of concerns of JsonContentHandler and JsonFieldTypeResolver. I now try to just use the existing code as much as possible. What do you think?

I have the feeling that now FieldTypeResolver is not an ideal name, because compared to JsonFieldTypeResolver it is doing something different. Should we still keep the name?

mduesterhoeft · 2018-10-04T20:24:23Z

spring-restdocs-core/src/main/java/org/springframework/restdocs/payload/ContentHandler.java

+	static ContentHandler forContent(byte[] content, MediaType contentType) {
+
+		try {
+			return new JsonContentHandler(content);


@wilkinsona I inlined the helper functions here because I could not keep them private in the interface. That is why I initially went for the factory class in the earlier version. This is now a little ugly. I could make ContentHandler an abstract class to keep the initial structure.

wilkinsona · 2018-11-21T10:40:27Z

Thanks for your patience, @mduesterhoeft.

I have the feeling that now FieldTypeResolver is not an ideal name, because compared to JsonFieldTypeResolver it is doing something different. Should we still keep the name?

I agree that it's not ideal. having a FieldTypeResolver interface that isn't implemented by JsonFieldTypeResolver does not feel right to me. What's actually exposed is the field type determination logic from the content handlers, rather than the type resolution logic from JsonFieldTypeResolver. As mentioned earlier, there's a subtle yet important difference between the two and I think it's important that different names are used to help to maintain that distinction.

Looking back at the discussion in Gitter, it's the logic that's currently in the content handler that you're interested in, so what's exposed is what you want. However, the internal structure isn't quite right. I'll need to take another look at this to see how best it can be achieved. It may be that we need to take a different approach.

See gh-549

* gh-549: Polish "Provide a public API for resolving the type of a field" Provide a public API for resolving the type of a field Rework JsonFieldTypeResolver to only discover the field types

wilkinsona · 2018-11-21T14:41:39Z

Thanks very much for your contribution, @mduesterhoeft. In the end I reworked JsonFieldTypeResolver to be something that just does discovery and renamed it accordingly. This meant that your proposal could drop on top of that pretty much unchanged. All the changes are now in master and will be in the 2.0.3 release.

Add public FieldTypeResolver.

abfe42f

This can be useful for extensions to reuse field type determination logic.

mduesterhoeft force-pushed the field-type-resolver branch from b319c32 to abfe42f Compare September 5, 2018 20:52

jlstrater approved these changes Oct 3, 2018

View reviewed changes

wilkinsona requested changes Oct 3, 2018

View reviewed changes

wilkinsona added this to the 2.0.3.RELEASE milestone Oct 3, 2018

wilkinsona added the type: enhancement Enhancement that adds a new feature label Oct 3, 2018

mduesterhoeft force-pushed the field-type-resolver branch from 94337bf to fdbb513 Compare October 4, 2018 06:55

Apply review hints.

61fe2fd

mduesterhoeft force-pushed the field-type-resolver branch from fdbb513 to 61fe2fd Compare October 4, 2018 08:39

wilkinsona requested changes Oct 4, 2018

View reviewed changes

mduesterhoeft force-pushed the field-type-resolver branch from d447fe0 to aa2041a Compare October 4, 2018 19:45

Keep separation of concerns of JsonContentHandler and JsonFieldTypeRe…

bc96ffd

…solver.

mduesterhoeft force-pushed the field-type-resolver branch from aa2041a to bc96ffd Compare October 4, 2018 20:10

mduesterhoeft commented Oct 4, 2018

View reviewed changes

wilkinsona added a commit that referenced this pull request Nov 21, 2018

Rework JsonFieldTypeResolver to only discover the field types

ada273b

See gh-549

wilkinsona pushed a commit that referenced this pull request Nov 21, 2018

Provide a public API for resolving the type of a field

4453fde

See gh-549

wilkinsona closed this in 64ea9c2 Nov 21, 2018

wilkinsona changed the title ~~Add public FieldTypeResolver.~~ Provide a public API for resolving the type of a field Nov 21, 2018

wilkinsona mentioned this pull request Dec 12, 2018

Diagnostics are poor when an attempt is made to extract a non-existent subsection of a payload #568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a public API for resolving the type of a field #549

Provide a public API for resolving the type of a field #549

mduesterhoeft commented Sep 5, 2018

mduesterhoeft commented Sep 6, 2018 •

edited

Loading

wilkinsona commented Sep 6, 2018

mduesterhoeft commented Sep 6, 2018

wilkinsona left a comment •

edited

Loading

wilkinsona Oct 3, 2018

wilkinsona Oct 3, 2018

wilkinsona Oct 3, 2018

wilkinsona Oct 3, 2018

wilkinsona left a comment •

edited

Loading

wilkinsona Oct 4, 2018

wilkinsona Oct 4, 2018

wilkinsona Oct 4, 2018

wilkinsona Oct 4, 2018

mduesterhoeft commented Oct 4, 2018

mduesterhoeft Oct 4, 2018

wilkinsona commented Nov 21, 2018

wilkinsona commented Nov 21, 2018

		@@ -0,0 +1,67 @@
		/*
		* Copyright 2014-2016 the original author or authors.

Provide a public API for resolving the type of a field #549

Provide a public API for resolving the type of a field #549

Conversation

mduesterhoeft commented Sep 5, 2018

mduesterhoeft commented Sep 6, 2018 • edited Loading

wilkinsona commented Sep 6, 2018

mduesterhoeft commented Sep 6, 2018

wilkinsona left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilkinsona left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mduesterhoeft commented Oct 4, 2018

Choose a reason for hiding this comment

wilkinsona commented Nov 21, 2018

wilkinsona commented Nov 21, 2018

mduesterhoeft commented Sep 6, 2018 •

edited

Loading

wilkinsona left a comment •

edited

Loading

wilkinsona left a comment •

edited

Loading