[go-experimental] Use builder pattern for requests

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇

[zippolyte]

Implement builder pattern to construct requests and pass all parameters. Similar construct as #4666

The motivation for this is to keep a client that will stay backward compatible in case some required parameters become optional which is not a breaking change in itself, but with the current implementation, it would change the method signature, thus introducing a breaking change in the client upon new generation.

Only path parameters are always required and any change would break the API itself, which is why I left them in the function signature.
cf https://swagger.io/specification/#parameterObject:

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true.

Additionally, this removes the use of the antihax/optional module for query parameters, as we don't need the explicit null functionality here.
cf #522 (comment)

Any feedback/suggestion would be much appreciated.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[antihax]

I recall there was a situation where we may need to specify path, qs, or body parameters as you could have a query or path parameter with the same name as a body parameter, etc: "A unique parameter is defined by a combination of a name and location."

The other projects using a builder pattern (Peal notably iirc) would specify this. @wing328 is this something to consider in this new design? I will see if i can find the example.

Update: Actually looks like everything has been refactored away from this, so maybe just ignore it.

[zippolyte]

Hmm that seems fair.
I would argue though that the current approach would have that name clash problem too, since the different kinds of parameters are in either the method signature, or the Opts object.

I actually remember having encountered this in our own specs, we had a path parameter that was name foo_bar and the endpoint accepted a body parameter of type FooBar. It resulted in a method like test(fooBar string, fooBar FooBar) so the code did not compile, and we just decided to change the name of the path parameter.

I'd be happy to hear about suggestions on how to workaround that though, and implement them in here.

[antihax]

It does, but so do many of the other clients in other languages. The solution i had seen (i thought was the perl version, but not anymore) was to use something like:

client.Function
.PathParameters(name, id)
.FormParameters(name, pet);

[zippolyte]

The problem with this one is that it still has the problem of generating client that cannot be backward compatible when for example a form parameter is added in the API.

This is the main problem I am trying to solve with the approach in this PR.

[zippolyte]

After giving more thought to this, I think the name clashing issue is not too big a problem anyway. It's easily fixable in the specification itself by changing the names of your path parameters for example, and there even is a x-codegen-request-body-name extension for the specific case of the body param name.
As you said also, it looks like things have been refactored away from the separation of parameter types.
With that said, what's your opinion on the approach I'm trying to take to solve the backward compatibility issue, while also retaining a decent UX ?

Also @wing328 or @jimschubert, I'm not too sure what's going on with the circleCI tests, can you advise here ?

Thanks all !

[wing328]

I've pushed a fix to the master. Please merge the latest master into this branch to resolve the build error. Sorry for the inconvenience caused.

[jimschubert]

AppVeyor looks like an unrelated issue with sockets closing in the C# client test.

[zippolyte]

Bumping, @antihax do you have any other feedback regarding this ?

[jimschubert]

I've skimmed this quickly (I'm by no means a Go expert), and I think it looks good.

I wonder if we should target the 4.3.x branch because it'll break existing client code (see diff on tests).

[zippolyte]

Yeah it will definitely break existing code. I felt like I could given this is the experimental template, but happy to target 4.3.x then.

[wing328]

@zippolyte for go-experimental (but not other generators), it's ok to have breaking changes in the master.

[jimschubert]

Awesome. Then yeah we can merge if there are no other concerns.

[zippolyte]

@jimschubert OK to merge then ?

[jimschubert]

Yeah, since it's been almost a week with no other concerns from contributors I think it's good to go.

[zippolyte]

Thanks for merging ! 🙇