Skip to content

Bash skip empty query param fix #3290

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 27, 2019
Merged

Bash skip empty query param fix #3290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27bdc1e
Bash: Update samples
ccouzens Jul 6, 2019
2b2ba98
Bash client: Fix query params seperation
ccouzens Jul 13, 2019
3dc421e
Bash: Skip query parameters with empty value
ccouzens Jul 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions modules/openapi-generator/src/main/resources/bash/client.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,18 +416,16 @@ build_request_path() {

local query_request_part=""

local count=0
for qparam in "${query_params[@]}"; do
if [[ "${operation_parameters[$qparam]}" == "" ]]; then
continue
fi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ccouzens I notice the following if condition in the code (line 455):

        if [[ "${collection_type}" == "" ]]; then
            local vcount=0
            for qvalue in "${parameter_values[@]}"; do
                if [[ $((vcount++)) -gt 0 ]]; then
                    parameter_value+="&"
                fi
                parameter_value+="${qparam}=${qvalue}"
            done

so what about using the following instead?

if [[ "${operation_parameters[$qparam]}" == "" ]]; then
...
done

Copy link
Contributor Author

@ccouzens ccouzens Jul 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wing328, thanks for reviewing :)

so what about using the following instead?

if [[ "${operation_parameters[$qparam]}" == "" ]]; then
...
done

I tried it. It has a different behaviour for when a blank parameter is passed in. For example invoking the client with username=

bash bash_keycloak/client.sh -v --oauth2-bearer "$access_token" --host "$api_host" realmUsersGet realm=demo_realm2 [email protected] username=

+isset creates the path /auth/admin/realms/demo_realm2/[email protected]=.

== "" creates the path /auth/admin/realms/demo_realm2/[email protected].

I think they're both wrong.
I think if the parameter is specified but blank, it should be creating the path /auth/admin/realms/demo_realm2/[email protected]&username=.

I'll see if I can update it to get the 3rd behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the commit I've just pushed, it has the 3rd behaviour
(/demo_realm2/[email protected]&username=)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using nullable in the spec to determine the desired behavior?
If nullable is set to true, then

/demo_realm2/[email protected]&username=

otherwise (default):

/demo_realm2/[email protected]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would nullable be the right thing to look for? Isn't an empty string different to null?

There is a field allowEmptyValue for query params.

Default value is false.

It defaults to not allowing empty parameters.

Use of this property is NOT RECOMMENDED, as it is likely to be removed in a later revision.

I think with that warning it doesn't make sense to implement the property. Presumably when (if) it gets removed query parameters will never be allowed to be empty. I'll make the change to drop empty parameters as you originally suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rebased and put in that change https://github.com/OpenAPITools/openapi-generator/compare/9ce50fb68e0f9729cdb23e4a764a38577193c122..3dc421e8b6684bc699556e6b46c24002bc476689#diff-9dfff4ad1f2cbb0764f6c97b86551379

There's a lot of noise due to rebasing off of master

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ccouzens Looks like the rebase failed.

Can you ping me via https://Gitter.im (ID: wing328) when you've time so that I can help you out and get this PR merged into master soon?

# Get the array of parameter values
local parameter_value=""
local parameter_values
mapfile -t parameter_values < <(sed -e 's/'":::"'/\n/g' <<<"${operation_parameters[$qparam]}")

if [[ -n "${parameter_values[*]}" ]]; then
if [[ $((count++)) -gt 0 ]]; then
query_request_part+="&"
fi
fi
{{#hasAuthMethods}}{{#authMethods}}{{#isApiKey}}{{#isKeyInQuery}}
if [[ ${qparam} == "{{keyParamName}}" ]]; then
if [[ -n "${parameter_values[*]}" ]]; then
Expand Down Expand Up @@ -508,6 +506,9 @@ build_request_path() {
fi

if [[ -n "${parameter_value}" ]]; then
if [[ -n "${query_request_part}" ]]; then
query_request_part+="&"
fi
query_request_part+="${parameter_value}"
fi

Expand Down
2 changes: 1 addition & 1 deletion samples/client/petstore/bash/.openapi-generator/VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3.0.0-SNAPSHOT
4.1.0-SNAPSHOT
39 changes: 32 additions & 7 deletions samples/client/petstore/bash/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# OpenAPI Petstore Bash client

## Overview

This is a Bash client script for accessing OpenAPI Petstore service.

The script uses cURL underneath for making all REST calls.
Expand Down Expand Up @@ -43,6 +44,7 @@ $ petstore-cli --host http://<hostname>:<port> --dry-run <operationid>
```

## Docker image

You can easily create a Docker image containing a preconfigured environment
for using the REST Bash client including working autocompletion and short
welcome message with basic instructions, using the generated Dockerfile:
Expand All @@ -59,6 +61,7 @@ is also available.
## Shell completion

### Bash

The generated bash-completion script can be either directly loaded to the current Bash session using:

```shell
Expand All @@ -72,10 +75,13 @@ sudo cp petstore-cli.bash-completion /etc/bash-completion.d/petstore-cli
```

#### OS X

On OSX you might need to install bash-completion using Homebrew:

```shell
brew install bash-completion
```

and add the following to the `~/.bashrc`:

```shell
Expand All @@ -85,27 +91,30 @@ fi
```

### Zsh
In Zsh, the generated `_petstore-cli` Zsh completion file must be copied to one of the folders under `$FPATH` variable.

In Zsh, the generated `_petstore-cli` Zsh completion file must be copied to one of the folders under `$FPATH` variable.

## Documentation for API Endpoints

All URIs are relative to */v2*

Class | Method | HTTP request | Description
------------ | ------------- | ------------- | -------------
*AnotherFakeApi* | [**testSpecialTags**](docs/AnotherFakeApi.md#testspecialtags) | **PATCH** /another-fake/dummy | To test special tags
*AnotherFakeApi* | [**123Test@$%SpecialTags**](docs/AnotherFakeApi.md#123test@$%specialtags) | **PATCH** /another-fake/dummy | To test special tags
*FakeApi* | [**createXmlItem**](docs/FakeApi.md#createxmlitem) | **POST** /fake/create_xml_item | creates an XmlItem
*FakeApi* | [**fakeOuterBooleanSerialize**](docs/FakeApi.md#fakeouterbooleanserialize) | **POST** /fake/outer/boolean |
*FakeApi* | [**fakeOuterCompositeSerialize**](docs/FakeApi.md#fakeoutercompositeserialize) | **POST** /fake/outer/composite |
*FakeApi* | [**fakeOuterNumberSerialize**](docs/FakeApi.md#fakeouternumberserialize) | **POST** /fake/outer/number |
*FakeApi* | [**fakeOuterStringSerialize**](docs/FakeApi.md#fakeouterstringserialize) | **POST** /fake/outer/string |
*FakeApi* | [**testBodyWithFileSchema**](docs/FakeApi.md#testbodywithfileschema) | **PUT** /fake/body-with-file-schema |
*FakeApi* | [**testBodyWithQueryParams**](docs/FakeApi.md#testbodywithqueryparams) | **PUT** /fake/body-with-query-params |
*FakeApi* | [**testClientModel**](docs/FakeApi.md#testclientmodel) | **PATCH** /fake | To test \&quot;client\&quot; model
*FakeApi* | [**testEndpointParameters**](docs/FakeApi.md#testendpointparameters) | **POST** /fake | Fake endpoint for testing various parameters
假端點
偽のエンドポイント
가짜 엔드 포인트
*FakeApi* | [**testEnumParameters**](docs/FakeApi.md#testenumparameters) | **GET** /fake | To test enum parameters
*FakeApi* | [**testGroupParameters**](docs/FakeApi.md#testgroupparameters) | **DELETE** /fake | Fake endpoint to test group parameters (optional)
*FakeApi* | [**testInlineAdditionalProperties**](docs/FakeApi.md#testinlineadditionalproperties) | **POST** /fake/inline-additionalProperties | test inline additionalProperties
*FakeApi* | [**testJsonFormData**](docs/FakeApi.md#testjsonformdata) | **GET** /fake/jsonFormData | test json serialization of form data
*FakeClassnameTags123Api* | [**testClassname**](docs/FakeClassnameTags123Api.md#testclassname) | **PATCH** /fake_classname_test | To test class name in snake case
Expand All @@ -117,6 +126,7 @@ Class | Method | HTTP request | Description
*PetApi* | [**updatePet**](docs/PetApi.md#updatepet) | **PUT** /pet | Update an existing pet
*PetApi* | [**updatePetWithForm**](docs/PetApi.md#updatepetwithform) | **POST** /pet/{petId} | Updates a pet in the store with form data
*PetApi* | [**uploadFile**](docs/PetApi.md#uploadfile) | **POST** /pet/{petId}/uploadImage | uploads an image
*PetApi* | [**uploadFileWithRequiredFile**](docs/PetApi.md#uploadfilewithrequiredfile) | **POST** /fake/{petId}/uploadImageWithRequiredFile | uploads an image (required)
*StoreApi* | [**deleteOrder**](docs/StoreApi.md#deleteorder) | **DELETE** /store/order/{order_id} | Delete purchase order by ID
*StoreApi* | [**getInventory**](docs/StoreApi.md#getinventory) | **GET** /store/inventory | Returns pet inventories by status
*StoreApi* | [**getOrderById**](docs/StoreApi.md#getorderbyid) | **GET** /store/order/{order_id} | Find purchase order by ID
Expand All @@ -133,25 +143,34 @@ Class | Method | HTTP request | Description

## Documentation For Models

- [$special[model.name]](docs/$special[model.name].md)
- [200_response](docs/200_response.md)
- [$special[modelName]](docs/$special[modelName].md)
- [200Response](docs/200Response.md)
- [AdditionalPropertiesAnyType](docs/AdditionalPropertiesAnyType.md)
- [AdditionalPropertiesArray](docs/AdditionalPropertiesArray.md)
- [AdditionalPropertiesBoolean](docs/AdditionalPropertiesBoolean.md)
- [AdditionalPropertiesClass](docs/AdditionalPropertiesClass.md)
- [AdditionalPropertiesInteger](docs/AdditionalPropertiesInteger.md)
- [AdditionalPropertiesNumber](docs/AdditionalPropertiesNumber.md)
- [AdditionalPropertiesObject](docs/AdditionalPropertiesObject.md)
- [AdditionalPropertiesString](docs/AdditionalPropertiesString.md)
- [Animal](docs/Animal.md)
- [AnimalFarm](docs/AnimalFarm.md)
- [ApiResponse](docs/ApiResponse.md)
- [ArrayOfArrayOfNumberOnly](docs/ArrayOfArrayOfNumberOnly.md)
- [ArrayOfNumberOnly](docs/ArrayOfNumberOnly.md)
- [ArrayTest](docs/ArrayTest.md)
- [Capitalization](docs/Capitalization.md)
- [Cat](docs/Cat.md)
- [CatAllOf](docs/CatAllOf.md)
- [Category](docs/Category.md)
- [ClassModel](docs/ClassModel.md)
- [Client](docs/Client.md)
- [Dog](docs/Dog.md)
- [DogAllOf](docs/DogAllOf.md)
- [EnumArrays](docs/EnumArrays.md)
- [EnumClass](docs/EnumClass.md)
- [Enum_Test](docs/Enum_Test.md)
- [Format_test](docs/Format_test.md)
- [EnumTest](docs/EnumTest.md)
- [FileSchemaTestClass](docs/FileSchemaTestClass.md)
- [FormatTest](docs/FormatTest.md)
- [HasOnlyReadOnly](docs/HasOnlyReadOnly.md)
- [MapTest](docs/MapTest.md)
- [MixedPropertiesAndAdditionalPropertiesClass](docs/MixedPropertiesAndAdditionalPropertiesClass.md)
Expand All @@ -164,20 +183,25 @@ Class | Method | HTTP request | Description
- [ReadOnlyFirst](docs/ReadOnlyFirst.md)
- [Return](docs/Return.md)
- [Tag](docs/Tag.md)
- [TypeHolderDefault](docs/TypeHolderDefault.md)
- [TypeHolderExample](docs/TypeHolderExample.md)
- [User](docs/User.md)
- [XmlItem](docs/XmlItem.md)


## Documentation For Authorization


## api_key


- **Type**: API key
- **API key parameter name**: api_key
- **Location**: HTTP header

## api_key_query


- **Type**: API key
- **API key parameter name**: api_key_query
- **Location**: URL query string
Expand All @@ -188,6 +212,7 @@ Class | Method | HTTP request | Description

## petstore_auth


- **Type**: OAuth
- **Flow**: implicit
- **Authorization URL**: http://petstore.swagger.io/api/oauth/dialog
Expand Down
Loading