[ObjC] Fix #2121, generated method names don't follow coding convention

[hideya]

Fixes #2121

Since I needed to reference both the untouched and the camelize names of the first argument in the mustache files, CodegenOperation.firstParamAltName has been introduced.

Now generated method names look like the following, e.g.:

-(NSNumber*) coffeeShopUpdateAllWithWhere: (NSString*) where
    data: (XXCoffeeShop*) data
    completionHandler: (void (^)(XXInlineResponse2001* output, NSError* error)) handler;

instead of:

-(NSNumber*) coffeeShopUpdateAllWithCompletionBlock :(NSString*) where 
     data:(XXCoffeeShop*) data 

    completionHandler: (void (^)(XXInlineResponse2001* output, NSError* error))completionBlock;

Note that the argument variable name for completionHandler has also been updated from completionBlock to handler, following the signature of a popular method of ModalWindowController:

    - (void)makeKeyAndOrderFront:(id)sender
                   modalToWindow:(NSWindow*)window
                      sourceRect:(NSRect)rect
               completionHandler:(void (^)(NSInteger result))handler;

Also, the name of -[ApiClient requestWithCompletionBlock:...] too has been updated to -[ApiClient requestWithPath:...] to follow the convention.

@wing328 I have tested the changes against my test cases but haven't made any changes in the test cases and the samples. Would you please tell me what I need to do for those?

[wing328]

Thanks for the PR.

Instead of introducing CodegenOperation.firstParamAltName, have you considered adding the firstParamAltName in Map<String, Object> postProcessOperations(Map<String, Object> objs)? Currently firstParamAltName is for ObjC client only and that's why I suggested earlier to add it in postProcessOperations for ObjC generator instead.

Please update the ObjC sample as mentioned in https://github.com/swagger-api/swagger-codegen/blob/master/CONTRIBUTING.md

To run the integration test, please do

cd samples/client/petstore/objc && mvn integration-test -rf :ObjcPetstoreClientTests

[hideya]

@wing328 Ah, now I see what you meant. Thank you!

UPDATE: I figured out the answer to the following question: operation.vendorExtensions.put("firstParamAltName", camelize(firstParamName));
I'll work on the samples next.

---

I'm working on postProcessOperations() and got a question, if you don't mind me asking...
Could you tell me how the ***** part in the following code should look like?
I couldn't figure it out how to put firstParamAltName per operation...

    @Override
    public Map<String, Object> postProcessOperations(Map<String, Object> objs) {
        Map<String, Object> operations = (Map<String, Object>) objs.get("operations");
        if (operations != null) {
            List<CodegenOperation> ops = (List<CodegenOperation>) operations.get("operation");
            for (CodegenOperation operation : ops) {
                if (!operation.allParams.isEmpty()) {
                    String firstParamName = operation.allParams.get(0).paramName;
                    ******.put("firstParamAltName", camelize(firstParamName));
                }
            }
        }
        return objs;
    }

Thank you in advance!

[hideya]

@wing328 I'm working on updating the Petstore sample and encountering an issue regarding binary support.

It looks that the latest petstore.json included two new endpoints whose response uses the following schema:

            "schema": {
              "type": "string",
              "format": "binary"
            }

For such endpoints, the current Obj-C codegen generates code like the following:

//
///
/// Fake endpoint to test byte array return by 'Find pet by ID'
/// Returns a pet when ID < 10.  ID > 10 or nonintegers will simulate API error conditions
///
/// @param petId ID of pet that needs to be fetched
/// 
///
/// @return SWGBinary*
-(NSNumber*) getPetByIdWithByteArrayWithPetId: (NSNumber*) petId
    completionHandler: (void (^)(SWGBinary* output, NSError* error)) handler;

where binary is mapped to a class SWGBinary, but there is no SWGBinary.h/m file generated, thus compilation of the sample ObjC client fails. I'm guessing that binary is introduced recently and ObjC codegen doesn't support it fully yet? (guessing so from this Issue: #1937) Or, I'm possibly doing something wrong, can you tell??

[wing328]

@hideya it's my oversight and the issue has been fixed by #2125 (merged into master). Please pull the latest master and rebase.

[hideya]

@wing328 thanks! I've updated the sample and the test files too. The updated tests run fine on my local machine.

While updating, I noticed an inconvenience of this PR's new method name generation proposal.
It happens when operationId includes the name of the first argument in such a way like getPetById.
In this particular case, the generated method looks like the following:

  [api getPetByIdWithPetId:[pet _id] completionHandler:^(SWGPet *output, NSError *error) {

FYI, it previously was:

  [api getPetByIdWithCompletionBlock:[pet _id] completionHandler:^(SWGPet *output, NSError *error) {

This may be a limitation of mechanically generating method names from operationIds for multiple languages that have different syntax characteristics and conventions.
I could include a special heuristics for ObjC where under some condition I don't put WithPetId and use getPetById as is (but this heuristic condition could be tricky.
What would be your take about this?

[hideya]

I'm not sure why the conversion of [pet _id] to NSString was needed in the original code...?

[wing328]

maybe _id was an integer/long before...

[wing328]

@hideya there's no way we can perfectly auto generate the method name to conform's to every language's style.

Personally I'm ok with your current approach. Would it meet the requirement for your use case? I mean do you have operationId similar to getPetById that's giving you a sub-optimal method name?

[hideya]

@wing328 Yeah, I also found similar cases like getPetById in our example, but I'm OK with the current approach too since I'd think more development will be done in Swift in future (I don't want to spend too much effort on ObjC, and including any heuristics could be nasty).

[wing328]

For perfection, we can introduce a vendor extension tag (e.g. x-objc-operationId) to use the value specified in x-objc-operationId if it's defined. I'll leave it up to you to implement it or not.

[hideya]

If you are fine with the current PR, then I'm fine too. If you'd rather like to have x-objc-operationId introduced, I'm happy to do that. Please let me know.

[wing328]

If you've time, please kindly work on it as this can serve as a workaround to use the "old" method name in Objc API client after upgrading to the latest version that includes your proposed change. In other words, this can make the upgrade backward compatible.

[hideya]

Sure thing! Would you point me an example code of vendor extension tag introduction in the codebase?

[wing328]

I think you can use it directly in the mustache template, e.g https://github.com/swagger-api/swagger-codegen/blob/ece56af43c2559c933a2e95bc283078c1ce88f91/modules/swagger-codegen/src/main/resources/Java/libraries/feign/api.mustache

[hideya]

I've updated this PR to support x-objc-operationId vendor extension tag.
Just in case, please let me summarise my understanding of the usage:

• the user who creates SwaggerSpec can include optional x-objc-operationId: tag per operation to explicitly specify the generated method name for that particular operation.
• for backward compatibility purpose, for example, the user can put x-objc-operationId: getPetByIdWithCompletionBlock to operation 'getPetById'
• to deal with the issue of unnatural generated method name discussed above, the user can put, for example, x-objc-operationId: getPetById to generate method -(NSNumber*) getPetById: (NSNumber*) petId ... instead of -(NSNumber*) getPetByIdWithPetId: (NSNumber*) petId ...

Does this sound right? If so, I think this PR is ready for re-review and for possible merge.

[fehguy]

Hi, this sounds good, but I would make sure that the java code will set the operationId if the x-objc-operationId does not exist. You may already have this in the code but just making sure that conceptually, it will work by default without any special extensions. They will just be sugar on the generation.

[hideya]

@fehguy thanks for the comment. Yes, the operationId gets set regardless of x-objc-operationId. As a matter of fact, Swift code generation for the PetStore sample is working fine, where x-objc-operationId is not set at all. Am I answering your question?

[wing328]

x-objc-operationId is used only in the template:

+-(NSNumber*) {{#vendorExtensions.x-objc-operationId}}{{vendorExtensions.x-objc-operationId}}{{/vendorExtensions.x-objc-operationId}}{{^vendorExtensions.x-objc-operationId}}{{nickname}}{{#hasParams}}With{{vendorExtensions.firstParamAltName}}{{/hasParams}}{{^hasParams}}WithCompletionHandler: {{/hasParams}}{{/vendorExtensions.x-objc-operationId}}{{#allParams}}{{#secondaryParam}}

so there should be no impact to other API clients.

[wing328]

@hideya The change looks very good to me. Thanks for the contribution.