Skip to content

Fix bug when using beneath path and wildcard together #714

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

Closed

Conversation

ddaaac
Copy link
Contributor

@ddaaac ddaaac commented Apr 6, 2021

Hi, I'm using restdocs so usefully 😀

I encountered following situation.

Here is the json body used in the request.

{
   "number":"123",
   "link":{
      "scheme":"scheme",
      "url":"url"
   },
   "chat":{
      "message":"send message",
      "id":"id",
      "thread":{
         "content":"thread content",
         "count":1
      }
   }
}

And here is the test code using beneathPath and wildcard.

val messageRequest = MessageRequest(
        number = "123",
        link = Link("scheme", "url"),
        chat = Chat("send message", "id", Thread("thread content", 1)),
)
mockMvc.perform(
        RestDocumentationRequestBuilders.get("/test")
                .contentType(MediaType.APPLICATION_JSON)
                .content(objectMapper.writeValueAsBytes(messageRequest))
)
        .andExpect(status().isOk)
        .andDo(MockMvcRestDocumentation.document(
                "test",
                requestFields(
                        fieldWithPath("number").type(JsonFieldType.STRING).description("a"),
                        subsectionWithPath("link").type(JsonFieldType.OBJECT).description("z"),
                        subsectionWithPath("chat").type(JsonFieldType.OBJECT).description("f"),
                ),
                requestFields(
                        beneathPath("*.thread").withSubsectionId("thread"),
                        fieldWithPath("content").type(JsonFieldType.STRING).description("thread message").optional(),
                        fieldWithPath("count").type(JsonFieldType.NUMBER).description("count").optional(),
                ),
        ))

And I saw error like below.

Caused by: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class java.lang.Object and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS)

This is because using wildcards will result in unmatched-pattern object field link being Extracted Field.Absent. Extracted Field.Absent is assigned to Object(), so serialization fail.
So I filter non-absent field.

@wilkinsona
Copy link
Member

Thanks for the proposal. Purely based on the example above, the usage of * doesn't feel quite right to me. It's intended for situations where there's a common structure beneath one or more unknown keys. In this case, the structure doesn't appear to be common with chat being the only entry that contains thread. It's this lack of common structure that's causing the failure. The error message could certainly be improved, but I'm not sure that the functionality should be changed.

Let's take a bit of a step back. What led to you using *.thread rather than chat.thread when documenting the thread subsection?

@wilkinsona wilkinsona added the status: waiting-for-feedback Feedback is required before progress can be made label Apr 6, 2021
@ddaaac
Copy link
Contributor Author

ddaaac commented Apr 7, 2021

It is the minimum code to reproduce.

{
   "number":"123",
   "link":{
      "scheme":"scheme",
      "url":"url"
   },
   "chat":{
      "message":"send message",
      "id":"id",
      "thread":{
         "content":"thread content",
         "count":1
      }
   },
   "chat2":{
     "message2":"send message",
     "thread":{
        "content":"thread content2",
        "count":2
      }
   },
   ...
}

There are several fields with the same structure as *.thread, and I wanted to document them at once. Is there a better way?


And even though this is intended, it would be nice if a user-friendly error message was displayed. Currently, the user should know that ABSENT is Object().

@wilkinsona
Copy link
Member

wilkinsona commented Apr 7, 2021

There are several fields with the same structure as *.thread, and I wanted to document them at once. Is there a better way?

Generally speaking, when something is difficult to document, that's also an indication that it will be difficult for clients to consume. If you have control over the format of the response, I would recommend grouping all of the chat… entries together. Something like this:

{
  "number":"123",
  "link":{
     "scheme":"scheme",
     "url":"url"
  },
  "chats":{
    "chat":{
       "message":"send message",
       "id":"id",
       "thread":{
          "content":"thread content",
          "count":1
       }
    },
    "chat2":{
      "message":"send message",
      "thread":{
         "content":"thread content2",
         "count":2
       }
    },
  }
}

You can then use chats.*.thread, chats.*.message, etc to document the structure of each chat. The more consistent structure will also make it easier for clients to map the response to objects, particularly in a type-safe language.

If changing the structure of the response isn't possible, you could document number, and link and then note that everything else should be treated as a chat. You could use relaxedResponseFields to document number and link and then perhaps beneathPath without a wildcard to focus on an individual chat and document that subsection of the response.

And even though this is intended, it would be nice if a user-friendly error message was displayed.

Yeah, I agree. I said above that "the error message could certainly be improved". I've opened #715 to do so.

@wilkinsona wilkinsona closed this Apr 7, 2021
@wilkinsona wilkinsona added status: declined Suggestion or change that we don't want to make at this time and removed status: feedback-provided Feedback has been provided status: waiting-for-triage Untriaged issue labels Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined Suggestion or change that we don't want to make at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants