Skip to content

Conversation

@GriffinSchneider
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Since #4916, path parameters that aren't in camel case have been broken.

When there are path parameters, code is generated like this:

        var path = "/{accountId}/thing"
        path = path.replacingOccurrences(of: "{account_id}", with: "\(accountId)", options: .literal, range: nil)

note that the path contains {accountId}, but we're trying to replace {account_id}, so it will never work. This is because the normalizePath function was camel-casing path parameters in the path.

This pullreq removes normalizePath, so the generated code now looks like this:

        var path = "/{account_id}/thing"
        path = path.replacingOccurrences(of: "{account_id}", with: "\(accountId)", options: .literal, range: nil)

and the replacement will work.

@jaz-ah
Copy link
Contributor

jaz-ah commented Mar 30, 2017

@GriffinSchneider I'm wondering what cases that code was originally added to solve - I think it would be worth adding in some tests for your cases and the original cases this code was meant to add to make sure we cover everything here.

@GriffinSchneider
Copy link
Contributor Author

GriffinSchneider commented Mar 31, 2017

Here's what I think happened:

The problem area in the swift3 templates is here: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/swift3/api.mustache#L107-L108

Before #4916, we used to be calling replacingOccurrences with paramName. According to #4898, using paramName in this way was causing bugs in other clients, because paramName is the 'normalized' (i.e. camel-cased) value, but the path we're trying to replace in contains parameter names that are not normalized. However, the swift3 client didn't have this bug, because someone added normalizePath as a workaround that would normalize all the parameters in the path so that they matched the normalized value in paramName.

Then, to fix #4898, paramName was replaced with baseName in the templates of many clients. In the swift3 client, this caused a bug, since the path was still being normalized but baseName contains the non-normalized parameter name. There are no non-camel-case path parameters in the petstore example specs, so the problem wasn't noticed and got merged.

So, my fix is simply to remove normalizePath, which was a bad workaround to begin with. Now the path contains non-normalized parameter names as expected, and we use the non-normalized baseName to replace them.

I will attempt to add a test that would have caught this issue, by including a non-camel-case path parameter in the petstore specs.

@GriffinSchneider
Copy link
Contributor Author

While trying to create a test for my original bug, I noticed a separate unrelated bug that was already breaking the swift3 tests.

The bug was in https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/swift3/_param.mustache

For enums of integer and long, that template was generating code that would work for integers or longs, but did not compile for enums of integer or long. c61f0cc fixes it. The swift3 tests now pass.

…dpoints-models-for-testing. This would have caused the Swift3 tests to be broken before 7387e49.
@GriffinSchneider
Copy link
Contributor Author

GriffinSchneider commented Mar 31, 2017

@jaz-ah OK, I've changed the tests so that they would have failed due to path parameter issues before this pull request.

@jaz-ah
Copy link
Contributor

jaz-ah commented Mar 31, 2017

@GriffinSchneider thx for the extra effort here! @wing328 +1

@wing328 wing328 merged commit 5de19e3 into swagger-api:master Apr 1, 2017
@wing328
Copy link
Contributor

wing328 commented Apr 1, 2017

@GriffinSchneider thanks for fixing the tests.

@jaz-ah thanks for reviewing the change.

davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
…er-api#5267)

* [Swift3] Fix bug where non-camel-case path params didn't work.

* [Swift3] Fix bug where int enums generated non-compiling code. Swift3 integration tests now pass.

* [Swift3] Add a non-camel-case path parameter to petstore-with-fake-endpoints-models-for-testing. This would have caused the Swift3 tests to be broken before 7387e49.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants