joffrey-bion / livedoc

A not-so-annotation-based documentation generator for REST and websocket services
MIT License
4 stars 2 forks source link

Livedoc generates duplicate ids #118

Closed ST-DDT closed 6 years ago

ST-DDT commented 6 years ago

Describe the bug

I have two methods that use the same context path but differ in the MediaType, but LiveDoc generates the same id for them. In the UI both are shown, but it stops working properly afterwards.

Expected behavior

IDs should be unique.

Screenshots

image Clicking on either will open them both.

image Afterwards the first entry will remain there even in other controllers.

Environment/Context

Livedoc-ID: #/apis/com.example.controller.TestController/post-test-target

Source:

@RestController
@RequestMapping("test")
public class TestController {

    @RequestMapping(path = "/{target}", method = RequestMethod.POST, consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
    public void upload(@PathVariable("target") long targetId, @RequestParam("file") MultipartFile file) {
    }

    @RequestMapping(path = "/{target}", method = RequestMethod.POST)
    public void upload(@PathVariable("target") long targetId, @RequestBody Object data) {
    }

}

Generated json (partial): https://gist.github.com/ST-DDT/b734b35d09648eb3a2fc33e78b9201c9

Suggestions

  1. Always append the hashCode of the method to the end of the id
  2. Also include the media type in the id
  3. Actively search for duplicate ids and add a disambiguator to them such as .1
joffrey-bion commented 6 years ago

Thanks for spotting this, I actually realized this flaw 2 weeks ago during the implementation of websocket support, but I still haven't come to a definitive solution. I'm really sorry not to have opened this issue myself when I first noticed it. It would have saved you some time.

I'm currently reworking the Livedoc IDs in the context of https://github.com/joffrey-bion/livedoc/issues/111, but it's actually nice to track this specific issue here.

I don't really like the solution with the hash (which is actually close to what JSONDoc originally used) because it clutters the URL, and I like my links clean. Also, to be able to easily reference things from Freemarker templates and descriptions (see https://github.com/joffrey-bion/livedoc/issues/111), it'd be nice to have short human-readable IDs.

The unicity is actually enforced by spring if we use their strategy to resolve method handlers: they use the path, the method, and the media types (which I had overlooked). Taking into account this whole combination should make the API operation IDs unique.

That being said, it would be annoying to see post-test-target-application-json if you have only a single method.

I have been considering for some time now using a LivedocIDProvider that could take a type/API/operation and generate an ID at runtime for it, thus resolving conflicts nicely.

This allows to avoid using fully qualified names for types and controllers (which are not supposed to appear in a client doc). Conflict would be resolved probably using number increments like you proposed, or with the last package name prepended to the simple class name.

For operations, we could get a nice compromise if we mention everything needed for unicity, but only if needed. For instance, we could use the path only most of the time, and if 2 operations use a different HTTP method for the same path, then we prepend the HTTP method. In your case, we would even append the media type to the ID. Maybe I'll keep the HTTP method all the time, I don't know yet.

The last problem I'm thinking about is how to retroactively augment the previously given IDs when a conflict is spotted. I believe I'll need a mutable structure instead of a String, but still serialize it as a String.

joffrey-bion commented 6 years ago

The problem has been solved, not by avoiding generation of duplicate IDs, but rather detecting it, giving a nice error message in the UI, and adding the possibility of overriding IDs in the @ApiOperation and similar.

I made this decision because I wanted to avoid dynamically changing the IDs of the doc elements, because it would have made it hard to reference them from documentation pages.