Skip to content
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

Question: Turn off unique operationId number suffixes, Add RequestMapping path to operationId #1224

Closed
zsavic opened this issue Mar 17, 2016 · 19 comments
Labels
Milestone

Comments

@zsavic
Copy link

zsavic commented Mar 17, 2016

I have a very simple use case with unique nicknames:
Two controllers:

@RestController
@RequestMapping("/test")
@Api(value="test")
public class TestController {

    @RequestMapping(method=RequestMethod.GET, value="/sayHello")
@ApiOperation(value="", nickname = "test_sayHello")
    public String sayHello(@RequestParam String name) {
        return "hello : " + name;
    }

}
@RestController
@RequestMapping("/test2")
@Api(value="test2")
public class TestController2 {

    @RequestMapping(method=RequestMethod.GET, value="/sayHello")
@ApiOperation(value="", nickname = "test2_sayHello")
    public String sayHello(@RequestParam String name) {
        return "hello : " + name;
    }

}

The operations can be called:
http://localhost:8080/test/sayHello?name=test2
http://localhost:8080/test2/sayHello?name=test3

Swagger Springfox currently generates this operationId's for this:
"operationId": "test_sayHello_1",
"operationId": "test2_sayHello_1",

1.) Why is Springfox adding the suffix "_1" here, as the nicknames are unique already?
IMHO it would be only necessary to add the suffixes, if the nicknames provided are not unique.

2.) Is it possible to have the RequestMapping path always as part of the operation name.
e.g. "operationId": "test_sayHello", for @RequestMapping("/test").sayHello()
Then the uniqueness of the operationId would be provided by default, as the compiler would guarantee no duplicate method names in the class.

If this is not possible now, is there a way to override the naming behaviour for operationIds to implement this on my own?

@zsavic zsavic closed this as completed Mar 17, 2016
@zsavic zsavic reopened this Mar 17, 2016
@jfiala
Copy link
Contributor

jfiala commented Mar 17, 2016

I ended up adding nicknames to every method prefixed with the request path (making them unique across the whole application, not only for the current controller request path).

However, currently the Springfox libraries still add the "_1" suffix to most methods (some methods are not suffixed for unknown reason).
This behaviour changes with every restart of the container, so sometimes the suffixes are there and sometimes not (moving between controllers/generated apiclients).
This means (senseless) refactoring after regenerating the client code.

@jfiala
Copy link
Contributor

jfiala commented Mar 17, 2016

I reverted back to 2.2.0, where the nicknames are picked up correctly without any suffixing of numbers and are only suffixed with the http verb.
So as of 2.2.0 the operationId's remain unchanged between restarts.

@dilipkrish
Copy link
Member

@jfiala/ @zsavic The generated method names are be predictable and unique. The functional tests verify this. Meaning it should NOT be changing when you restart services etc. If it does its a problem.

Now the reason its adding _1 suffix to methods annotated with the nickname specified is more than likely that you have more than one docket in play.

For issues like this it would be great if you could create a simple reproducing example so that its easier for me to diagnose. A good place to start is to use the springfox-demos and get it to fail like you're seeing happen.

Also 2.2.0 is probably one of the more buggy versions. I'd recommend sticking to 2.4.0

@dilipkrish dilipkrish added this to the 2.5.0 milestone Mar 17, 2016
@jfiala
Copy link
Contributor

jfiala commented Mar 17, 2016

Regarding the "_1": this in fact appears with one Docket as soon as I have more than one controller on the path with nicknames attached to the method names. So it has nothing to do with multiple Dockets.

It has only to do with multiple controllers, as soon as they have the same signature (here: sayHello(String name)! If I change the method name, the nickname-suffix disappears (e.g. sayHello2(String name) or sayHello(String name2)!

With one controller, there is no "_1" suffix. As soon as there are two controllers with a method with the same signature, there is a suffix at the end of the operationId:

my-controller: operationId: mypath_sayHello
test-controller: operationId: test_sayHello_1

@RestController
@RequestMapping("/test/test")
@Api(value="test")
public class TestController {

    @RequestMapping(method=RequestMethod.GET, value="/sayHello")
    @ApiOperation(value="", nickname = "test_sayHello")
    public String sayHello(@RequestParam String name) {
        return "hello : " + name;
    }
}

@RestController
@RequestMapping("/test/mypath")
@Api(value="mypath")
public class MyController {
    @RequestMapping(method=RequestMethod.GET, value="/sayHello")
    @ApiOperation(value="", nickname = "mypath_sayHello")
    public String sayHello(@RequestParam String name) {
        return "hello : " + name;
    }
}

Since usually methods have have identical signatures in different controllers (e.g. public MyObject getbyId(Long id)), this results in "_1" suffixed to unique nicknames, and its resulted in "_7" suffixed in one of my controllers (as I have 7 controllers with identical method signatures...).

@jfiala
Copy link
Contributor

jfiala commented Mar 17, 2016

I've been able to track this down comparing with the boot example - this was causing it in my Spring XML configuration:

<mvc:annotation-driven />

This had no other effect than somehow causing cycles in the nickname calculation.
I removed it, and now method names are generated as expected.

However, I have still to add nickname to each method, otherwise they are suffixed, although they are unique in their scope!
Is there a specific reason why methodnames from different controllers with different paths are suffixed?

@jfiala
Copy link
Contributor

jfiala commented Mar 17, 2016

One additional thing I noted when switching between 2.4.0 and 2.2.0 (pls let me know if I should post a separate question):
In 2.4.0 the @Api(value="mycontrollername") is no longer used as tag name, instead the controller class name is always used as a tag name. Is it still possible to change the tag name?

@jfiala
Copy link
Contributor

jfiala commented Mar 17, 2016

To sum things up (my test results with my configuration):
1.) nicknames are working only if they are unique across all controllers of the application (also for multiple Dockets!).
So I think this should be changed, to scope the nicknames per controller/tag, not per application!?

2.) If no nicknames are used, the method names are also checked for duplicates across the whole application.
This should be also scoped per controller/tag (as they are unique already themselves).

3.) There is no way to get rid of the "UsingVERB" suffix without adding nicknames currently. I think it would be nice to be able to configure this in the Docket or so?

Please let me know if I should add an example PR in the demos?

@dilipkrish
Copy link
Member

@jfiala that is by design. That is what the spec says

Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is recommended to follow common programming naming conventions.

So 1 and 2 are as designed.

Regarding #3 you should be able to implement your own OperationBuilderPlugin and override the codegenMethodNameStem like it does here

@jfiala
Copy link
Contributor

jfiala commented Mar 18, 2016

I found this interpretation regarding the spec:
OAI/OpenAPI-Specification#381
"webron: The 'basic' limitation would be within the same Swagger description. The real challenge is how it would behave when referencing external files."

In fact, the term API refers to per API documentation/Swagger description provided, i.e. per Docket.

I added a question there to clarify if the scope interpretation can be narrowed down further.
But the suffixing of operationId across Dockets currently seems to be interpreted too broadly (and doesn't make much sense to me, as the API docs are provided for different consumers, who don't know anything of the other API documentation endpoints - Suffixes appearing there would probably confuse the consumers).

However, until these things are clarified:
I see two options to get rid of the suffixed in the client code (without using nicknames)

  • Option A: Override the assembling of the operationId to the simple tag scoping I suggested above (operationId is unique in the scope of a controller class/tag).
  • Option B: Change Swagger Codegen to ignore the operationId and assemble the method based on the Request Path.

What do you think, does Option A make sense to you?
Additionally, if I implement the OperationBuilderPlugin is there a method later in the chain that will overrule my nickname and add suffixes again?

@dilipkrish
Copy link
Member

From what I remember earlier Option A is hard to implement such that the operation names are consistent and predictable and not dependent on spring bean loading order etc.

@jfiala
Copy link
Contributor

jfiala commented Mar 22, 2016

I understand this, so let the operationId unique for the sake of the current interpretation of the OpenAPI spec. I'll adapt Swagger-Codegen to ignore the operationId, so everything is fine at the client code.

@rkaltreider
Copy link

rkaltreider commented Jun 29, 2016

In case anyone else is running into this. I experienced this same behavior due to having two dockets. I have a customer docket with a filtered view of the API and an admin docket with all endpoints listed. Unfortunately the docs generated with the _1 are on the customer api-docs group. This leads to silly looking SDK methods with a 1 at the end. I read all the suggestions, but i ended up with something a little different. I decided to use controller advice and attach it to the Swagger2Controller to alter the document before its written to the response. This gives me full control of the output. I felt is was pretty elegant until something better is added. This can obviously be extended to do more, but its working for me at the moment.

@ControllerAdvice(assignableTypes = Swagger2Controller.class)
public class ApiDocsAdvice implements ResponseBodyAdvice<Object> {

    @Autowired
    private ObjectMapper objectMapper;

    @Override
    public boolean supports(MethodParameter returnType, Class<? extends HttpMessageConverter<?>> converterType) {
        return true;
    }

    @Override
    public Object beforeBodyWrite(Object body, MethodParameter returnType, MediaType selectedContentType, Class<? extends HttpMessageConverter<?>> selectedConverterType, ServerHttpRequest request, ServerHttpResponse response) {
        try {
            JsonNode document = this.objectMapper.readTree(((Json)body).value());
            this.sanitize(document, "operationId");
            return document;
        } catch (Exception e) {
            return body;
        }
    }

    private void sanitize(JsonNode parent, String fieldName) {
        if (parent.has(fieldName)) {
            String text = ((ObjectNode) parent).get(fieldName).textValue();
            if(null != text) {
                int pos = text.indexOf("_");
                if(pos > -1) {
                    ((ObjectNode) parent).set(fieldName, new TextNode(text.substring(0, pos)));
                }
            }
        }

        for (JsonNode child : parent) {
            this.sanitize(child, fieldName);
        }
    }

}

@AndriiPlotnikov
Copy link

If anyone else is having this problem or similar

the simplest solution for me was to override operationName Generator which was causing the issue.
The reason for causing it was custom plugin that took operationBuilder and called build, effectively increaing counter (_1 suffix)

the solution was to provide custom counter

@Component
@Primary 
public class CustomOperationNameGenerator implements OperationNameGenerator {
    @Override
    public String startingWith(final String prefix) {
        return prefix;
    }
} 

You can also implement custom caching:
autowire it in class that calls operationBuilder.build and decrease counter if you made call you that increases counter. After that throw exception if counter >0 second time you call. This will ensure you don't have duplicate methods and will fail on startup.

I don't know why this was not done, since duplicate operationId breaks the swagger, should it not be responsibility of the developer to fix it?

@dilipkrish
Copy link
Member

@sarief It has been implemented, You need to subclass CachingOperationNameGenerator instead

@AndriiPlotnikov
Copy link

@dilipkrish oh, good. I must have old version then, thanks for pointer 👍

@uce
Copy link

uce commented Jan 18, 2020

@dilipkrish What do you think about allowing to configure a OperationNameGenerator per DocumentationContext#getGroupName? Is there a work around to achieve this? I think this would solve the problems mentioned in this thread while still avoiding conflicts within a single Docket.

The proposed work around avoids suffixes like _1 at the cost of allowing conflicts within a Docket with duplicate IDs which can lead to issues during code generation.

@wouterh-dev
Copy link

I wound up going with the following workaround which makes the generated names unique within a docket:

  /**
   * Reimplement CachingOperationNameGenerator with enhanced logging, to include name of the docket
   * in the log if duplicate names are generated
   */
  public static class CustomCachingOperationNameGenerator implements OperationNameGenerator {

    private static final Logger LOG = LoggerFactory
        .getLogger(CustomCachingOperationNameGenerator.class);
    private Map<String, Integer> generated = newHashMap();
    private final String groupName;

    public CustomCachingOperationNameGenerator(String groupName) {
      this.groupName = groupName;
    }

    @Override
    public String startingWith(String prefix) {
      if (generated.containsKey(prefix)) {
        generated.put(prefix, generated.get(prefix) + 1);
        String nextUniqueOperationName = String.format("%s_%s", prefix, generated.get(prefix));
        LOG.info("Generating unique operation for group {} named: {}", groupName,
            nextUniqueOperationName);
        return nextUniqueOperationName;
      } else {
        generated.put(prefix, 0);
        return prefix;
      }
    }
  }

  /**
   * Replace the ApiOperationReader with a custom version that ensures the OperationNameGenerator is
   * unique for each docket
   */
  @Component
  @Primary
  @Qualifier("default")
  public static class CustomApiOperationReader implements OperationReader {

    private final Thread currentThread;
    private final ApiOperationReader delegate;
    private final Map<String, OperationNameGenerator> operationNameGeneratorMap = new HashMap<>();

    private String currentGroupName;

    @Autowired
    public CustomApiOperationReader(
        DocumentationPluginsManager pluginsManager) {
      this.currentThread = Thread.currentThread();
      this.delegate = new ApiOperationReader(pluginsManager, this::generateName);
    }

    public String generateName(String prefix) {
      if (Thread.currentThread() != currentThread) {
        throw new UnsupportedOperationException("CustomApiOperationReader is not thread safe");
      }

      return operationNameGeneratorMap
          .computeIfAbsent(currentGroupName,
              CustomCachingOperationNameGenerator::new)
          .startingWith(prefix);
    }

    @Override
    public List<Operation> read(RequestMappingContext outerContext) {
      if (Thread.currentThread() != currentThread) {
        throw new UnsupportedOperationException("CustomApiOperationReader is not thread safe");
      }

      this.currentGroupName = outerContext.getDocumentationContext().getGroupName();
      return delegate.read(outerContext);
    }
  }

@wouterh-dev
Copy link

@dilipkrish Could this be reopened? In my opinion it is a desirable feature to be able to have the CachingOperationNameGenerator only cache within a single docket, since duplicate operation names in multiple dockets is not a problem, and leads to very unexpected results when exposing several subsets of APIs in separate dockets.

@freshgeek
Copy link

This operation will slow down the startup of the environment if the code is generated automatically
for example :

2021-02-07 10:08:02.499  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: pageUsingPOST_1
2021-02-07 10:08:02.502  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: findByIdUsingGET_1
2021-02-07 10:08:02.503  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: deleteUsingPOST_1
2021-02-07 10:08:02.504  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: insertUsingPOST_1
2021-02-07 10:08:02.505  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: updateUsingPOST_1
2021-02-07 10:08:02.508  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: listUsingPOST_1
2021-02-07 10:08:02.524  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: pageUsingPOST_2
2021-02-07 10:08:02.527  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: findByIdUsingGET_2
2021-02-07 10:08:02.529  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: deleteUsingPOST_2
2021-02-07 10:08:02.530  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: insertUsingPOST_2
2021-02-07 10:08:02.531  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: updateUsingPOST_2
2021-02-07 10:08:02.534  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: listUsingPOST_2
2021-02-07 10:08:02.558  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: pageUsingPOST_3
2021-02-07 10:08:02.561  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: findByIdUsingGET_3
2021-02-07 10:08:02.562  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: deleteUsingPOST_3
2021-02-07 10:08:02.564  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: insertUsingPOST_3
2021-02-07 10:08:02.565  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: updateUsingPOST_3
2021-02-07 10:08:02.569  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: listUsingPOST_3

........
 
2021-02-07 10:08:16.210  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: pageUsingPOST_187
2021-02-07 10:08:16.229  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: findByIdUsingGET_185
2021-02-07 10:08:16.245  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: deleteUsingPOST_187
2021-02-07 10:08:16.278  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: insertUsingPOST_187
2021-02-07 10:08:16.310  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: updateUsingPOST_187
2021-02-07 10:08:16.343  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: listUsingPOST_187
2021-02-07 10:08:16.387  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: pageUsingPOST_188
2021-02-07 10:08:16.405  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: findByIdUsingGET_186
2021-02-07 10:08:16.422  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: deleteUsingPOST_188
2021-02-07 10:08:16.453  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: insertUsingPOST_188
2021-02-07 10:08:16.485  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: updateUsingPOST_188
2021-02-07 10:08:16.520  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: listUsingPOST_188
2021-02-07 10:08:16.561  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: pageUsingPOST_189
2021-02-07 10:08:16.579  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: findByIdUsingGET_187
2021-02-07 10:08:16.597  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: deleteUsingPOST_189
2021-02-07 10:08:16.629  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: insertUsingPOST_189
2021-02-07 10:08:16.662  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: updateUsingPOST_189
2021-02-07 10:08:16.696  INFO 24592 --- [           main] .d.s.w.r.o.CachingOperationNameGenerator : Generating unique operation named: listUsingPOST_189

You can see that it took more than ten seconds in the middle

These methods are generated by code generator and have similar crud names in different controllers. Do you consider that this scan has performance problems

How can I solve it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants