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

[Ruby] Client: fix base_url when no server_operation_index is defined #15162

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

Confusion
Copy link
Contributor

@cliffano, @zlx, @autopp

As discussed in #7415 (comment), it seems unlikely the code was correct.

server_operation_index is a hash table. In Ruby, hash[key] will return the value associated with key. If key is absent, nil is returned. Because that is sometimes undesirable, there is also hash.fetch(key), which raises an error if the key is absent. It also allows you to specify a default to fall back on: hash.fetch(key, default) will return default if the key is absent.

So, since not all users will specify a 'server per operation' (or at least: I'm not), the old code would usually set index to the server_index, which is initialized to 0. The subsequent if index == nil will usually return false (0 != nil in Ruby), after which the server_url call on line 177 constructs the url based on the server_operation_variables and operation_server_settings, assuming we are dealing with the case where a server per operation is configured. The case where the url should be constructed from scheme, host, etc. is only called if either server_index is explicitly set to nil or the key operation is explicitly associated with the value nil in the server_operation_index hash table, both of which seem inappropriate.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date.sh
    
    Commit all changed files.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

As discussed in OpenAPITools#7415 (comment), it seems unlikely the code was correct.

server_operation_index is a hash table. In Ruby, `hash[key]` will return the value associated with `key`. If key is absent, `nil` is returned. Because that is sometimes undesirable, there is also `hash.fetch(key)`, which raises an error if the key is absent. It also allows you to specify a default to fall back on: `hash.fetch(key, default)` will return `default` if the key is absent.

So, since not all users will specify a 'server per operation' (or at least: I'm not), the old code would usually set `index` to the `server_index`, which is initialized to 0. The subsequent `if index == nil` will usually return false (`0 != nil` in Ruby), after which the `server_url` call on line 177 constructs the url based on the `server_operation_variables` and `operation_server_settings`, assuming we are dealing with the case where a server per operation is configured. The case where the url should be constructed from `scheme`, `host`, etc. is only called if either `server_index` is explicitly set to `nil` or the key `operation` is explicitly associated with the value `nil` in the `server_operation_index` hash table, both of which seem inappropriate.
@wing328
Copy link
Member

wing328 commented Apr 21, 2023

LGTM but I've not tested it locally.

Let's give it a try.

@wing328 wing328 merged commit 2679819 into OpenAPITools:master Apr 21, 2023
@wing328 wing328 added this to the 6.6.0 milestone Apr 21, 2023
ColeMurray added a commit to ColeMurray/openapi-generator that referenced this pull request May 23, 2023
@ColeMurray
Copy link

This commit breaks functionality to allow switching between servers. Specifically, this is useful for fetching from dev/prod.

Example usage:

openapi: "3.0.3"
info:
title: Service
description: Service responsible for being a service
version: "1.0"
servers:

paths:
/v1/endpoint:
$ref: "./api/v1/service.yaml#/~services"

Usage:

Client.configure do |config|
config.access_token = @http_token
config.server_index = 2
end
With this change, we are no longer able to switch between our different servers

@Confusion
Copy link
Contributor Author

The previous version didn't work for anyone outside of your use case, so reverting this change doesn't seem like the best thing for the general good. A fix would be to replace:

index = server_operation_index[operation]

by

index = server_operation_index[operation || server_index]

I didn't understand how the server_index was being used, as it's not being set anywhere in the code base and there are no examples.

@ckoegel ckoegel mentioned this pull request Jul 20, 2023
6 tasks
@wing328
Copy link
Member

wing328 commented Jul 25, 2023

#16144 has been merged into master.

Please pull the latest master to give it a try. (or use the snapshot version)

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