Make extension of spring-restdocs easier

[kellen]

I've just started using spring-restdocs, but I already have several use cases where I would have liked to more easily extend the existing classes, but have been forced into some java-gymnastics to be able to implement my changes.

Some examples:

1. I want to be able to add LinkDescriptors, FieldDescriptors, and ParameterDescriptors in several steps
2. I want to be able to override already-added descriptors by name
3. I don't want AsciidoctorWriter to write a $ before bash command since this makes copy-pasting the command difficult.
4. I want CurlDocumentation to use a bash variable instead of the hostname so that a user of the documentation can export HOST=foo.bar and then curl "${HOST}/api/"

In order to do (1) I can implement ResultHandler, number (2) requires implementing a Comparator on the descriptors, which necessitates me putting code in org.springframework.restdocs.hypermedia, org.springframework.restdocs.payload, and org.springframework.restdocs.request since the accessors on getRel(), getPath(), and getName() have no access modifiers nor do the constructors. (3) requires overriding AsciidoctorWriter, but in order to get this functionality into the curl documentation I must also override then entirety of CurlDocumentation and SnippetWritingResultHandler. (4) also essentially copy-pasting the contents of CurlDocumentation...

All this to say that it would be (at least for me) useful to have at least protected access to the constructors and accessors so that extending the base functionality wasn't so painful.

[ghost]

I agree with the suggestion, and have run into a set of related use cases. Not sure it is best to create a new ticket, or append to this one. In general, allowing for extensible design, such as protected fields, would be a huge improvement. So far I am extremely happy with the features of the library and giving consumers more "seams" to extend would go a long way to spread adoption.

One of the things we do in our APIs is provide support for multiple versions of an API at the same time. To avoid the natural copy/paste style inherent to Spring-MVC we generally use AOP techniques to manage the version boundaries (e.g. annotations). This means that when we run our tests using mock mvc we are using a JUnit rule to cause the test to be evaluated multiple times, once for each version that we dictate the test to be supported for. This is particularly useful when an API is present in multiple versions, but does not change its behavior, which is the most common case for us.

To customize restdocs to our needs, I would love to be able to do two things:

Custom Output File Resolver

The parameterized formats such as {step} are great, but not quite flexible enough to handle more sophisticated test conventions. OutputFileResolver would be a great seam to customize along, but it is package-private and the SnippetWritingResultHandler doesn't support injecting a different resolver implementation. Perhaps a simple interface would do the trick, but it may also require a commensurate adjustment to RestDocumentationContext.

Custom Result Handlers

One extension that I would like to make is registering a custom ResultHandler within the RestDocumentationResultHandler. In my case, I'd like to output another adoc document that indicates which versions of the API are supported by a particular endpoint. This would do a lookup of my custom AOP-driven metadata. Unfortunately, the class RestDocumentationResultHandleris public but is not extensible because the list of delegates is private, rather that protected. Alternatively, a public method to append the delegate would be sufficient.

Would you recommend that we look to contribute pull-requests for these enhancement ideas? If so, I want to make sure it aligns with the architectural direction intended.

[wilkinsona]

@kellen Thanks for opening the issue, and apologies for the delayed response. @mhuffman-r7 This is the right place for your comments. Thanks for adding to the discussion.

My approach thus far has been to deliberately limit the visibility and extensibility of classes. That's not because I don't want the library to be extensible, but because I want it to be extensible in the right way and in the right places. The feedback above is vital to doing that.

Pull requests for improving the extensibility would definitely be welcome, but I'd like to have the general approach to extensibility nailed down before people spend time on changes the may be going in the wrong direction. I'd like to understand the need for some of the extensibility improvements suggested above. To that end, here's some questions and comments on what's been suggested thus far:

I want to be able to add LinkDescriptors, FieldDescriptors, and ParameterDescriptors in several steps

@kellen What do you mean by "in several steps"? Perhaps you can provide a concrete example of what you'd like to be able to do?

I want to be able to override already-added descriptors by name

@kellen I don't understand the need for this. There aren't any descriptors added by default. Why are you adding them, if it means that they then need to be overridden?

I don't want AsciidoctorWriter to write a $ before bash command since this makes copy-pasting the command difficult.

Another way to address this would be to provide a configuration option for the prefix.

I want CurlDocumentation to use a bash variable instead of the hostname so that a user of the documentation can export HOST=foo.bar and then curl "${HOST}/api/"

This could, perhaps, also be addressed by a RequestPostProcessor abstraction that mirrors what's already in place for responses.

Custom output file resolver

The intention has always been for the output to be pluggable as I wanted to support different output formats. That rather dropped off the radar, unfortunately, when it became apparent that Asciidoctor was pretty much the only option that had the ability to import snippets into a document.

I think this is a good suggestion, and it also seems to get to the crux of the current difficulties. It's tempting to add everything that should be configurable/pluggable to RestDocumentationContext, but I think that could get rather messy, rather quickly. It also already makes testing a bit of a pain and it would be nice not to add to that. In short, this one's going to take some thought.

Custom result handlers

This one seems pretty straightforward. Adding the current default handlers in a protected method that a subclass can override would seem like a good, low-impact solution. You can then call super and augment the default handlers, or avoid calling super and take complete control.

[ne-kellen]

What do you mean by "in several steps"?

I have an API in which there are always certain fields and links. For example, I always provide a links field and always provide a self and discovery link. Each call to the API has additional fields and links.

So, what I want is to be able to do document(...).withLinks(defaultLinks()).withLinks(customLinks())

Due to the way the delegates are handled in RestDocumentationResultHandler, if I call withLinks with only a partial description of the links on the response, and the resulting LinkSnippetResultHandler will throw an exception during processing.

Why are you adding them, if it then means that they then need to be overridden?

There's lots of endpoints with similar fields, so I want to be able to define a global set of fields, links, and parameters but then override these if necessary. e.g.

public LinkDescriptor[] defaultLinks() {
    return new LinkDescriptor[] {
        linkWithRel("foo").description("bar"),
        // ...
    };
}

// ...

document(...).withLinks(defaultLinks()).withLinks(linkWithRel("foo").description("BAR"));

[wilkinsona]

@ne-kellen Got it. Thanks.

One way to tackle this without requiring you to extend anything would be for withLinks to be additive. LinkSnippetResultHandler already stores the descriptors in a Map keyed by rel. They're added to the map in the order that they're provided so the last descriptor for a particular rel will override any that have come before it.

I think you can do this today, albeit in a rather clunky manner, by combining the default links with the custom links yourself and then passing the combined array to a single call to withLinks.

[ne-kellen]

Right. My solution was to implement ResultHandler and change the definition of `withLinks:

    protected Set<LinkDescriptor> linkDescriptors = new HashSet<>();

    public RestDocumentationResultHandler withLinks(LinkDescriptor... descriptors) {
        Collections.addAll(linkDescriptors, descriptors);
        return this;
    }

Then I only add the delegate when handle() is called:

    @Override
    public void handle(MvcResult result) throws Exception {
        List<ResultHandler> delegates = new ArrayList<>();
        // ...
        if (!linkDescriptors.isEmpty()) {
            delegates.add(documentLinks(this.outputDir, null, linkDescriptors));
        }
        //...
        for (ResultHandler delegate : delegates) {
            delegate.handle(result);
        }
    }

[ghost]

@wilkinsona For "Custom output file resolver" that sounds great. I do see that the context is going to be a bit tricky, as its using a thread-local-like design pattern. In my experience these patterns are tricky to extend, particularly because of the static singleton calls.

As far as the "Custom result handlers", agree it should be pretty easy. The only thing that may not be clean is the handling of the output directory. An adapter may be desirable here so that the handler registered with the RestDocumentationResultHandler inherits its output directory.

[marceloverdijk]

I haven't looked at the source code but I assume the snippets are generated using some template engine? It would be nice to change these templates per project. This way we could remove e.g. the $ from the curl-template as being mentioned earlier.

[wilkinsona]

@marceloverdijk There's no template engine at the moment, largely because I want to minimise dependencies as much as possible. It's an interesting idea, though. Thank you.

[marceloverdijk]

@wilkinsona OK I understand. I think using templates could give a lot of of flexibility and possibilities for customizations.

[wilkinsona]

@marceloverdijk Agreed. It'd probably provide an answer for quite a few of the open issues. I plan to look into doing something optional, i.e. a template engine can be used if one's on the classpath, otherwise it falls back to the current behaviour.

[marceloverdijk]

@wilkinsona yes that sound reasonable to do!

[kellen]

One super-simple templating solution would be to use commons' StrSubstitutor, though I suppose that's less spring-y than using SpEL...

[wilkinsona]

The latest snapshots are now using JMustache and templates to produce the snippets. This takes care of number 3 from the original list of examples as you can now provide your own curl-request.snippet template that removes the $ prefix.

[wilkinsona]

@mhuffman-r7 @kellen @marceloverdijk (and, of course, anyone else who's interested): the latest snapshots use a WriterResolver to determine the Writer that should be used to write a snippet. You can plug in your own implementation when you're building the MockMvc instance:

this.mockMvc = MockMvcBuilders.webAppContextSetup(this.context)
        .apply(documentationConfiguration()
                .writerResolver(new CustomWriterResolver()))
        .build();

I've also pushed up a branch that reworks the API. One of the main changes is that you can now include any implementation of Snippet when calling document as they're passed in as varags to the document call rather than being provided by the various withXXX methods:

this.mockMvc.perform(get("/"))
            .andExpect(status().isOk())
            .andDo(document("index-example",
                    links(
                            linkWithRel("notes").description("…"),
                            linkWithRel("tags").description("…")),
                    responseFields(
                            fieldWithPath("_links").description("…")),
                    yourCustomSnippet()));

Also, you now have complete control over the default snippets:

this.mockMvc = MockMvcBuilders.webAppContextSetup(this.context)
        .apply(documentationConfiguration().snippets()
            .withDefaults(curlRequest(), yourCustomSnippet()))                      
        .build();

These are breaking changes, so they're not being made lightly (and I'm not yet 100% that they will be made). That said, I think they're a significant improvement over what was in M1 but I'd like feedback from others before proceeding. If you have some time, please check out the branch and let me know what you think.

Please note that I've yet to open up the constructors and accessors on the various Snippet implementations as requested above by @kellen but it's on the cards.

[marceloverdijk]

Hi @wilkinsona

I'm going on holiday this week so unfortunately I wontt have time to look into it now. Looking at the above the varargs api change looks really nice and indeed offers a lot of flexibility to attache custom snippet writers. This looks very promising!