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

Discussion: Custom query params #99

Closed
wants to merge 2 commits into from

Conversation

joshco
Copy link

@joshco joshco commented Mar 12, 2016

This is just for discussion, merge not requested. Re issue #97

Allow the caller to set custom query parameters when fetching a link

events_link=api._links['events']
events_link.query_params= { filter: "modified_date gt '2016-01-01'" }
events=events_link['items']

events_link=api._links['events']
events_link.query_params= { filter: "modified_date gt '2016-01-01'" }
events=events_link['items']
@dblock
Copy link
Collaborator

dblock commented Mar 13, 2016

HAL wants you to be much more strict with query parameters, ie. you should only expand those that are decalred. See #84 for a discussion on this, I'd like to support both ways, but I think all it needs is to relax the existing implementation where we only expand the existing template with known parameters.

@dblock
Copy link
Collaborator

dblock commented Mar 13, 2016

See hannesg/uri_template#14 and #92 for a suggested implementation.

@dblock
Copy link
Collaborator

dblock commented Mar 13, 2016

I am going to close this because the implementation in #92 is a lot cleaner, IMO, @joshco see if it accomplishes the same thing as what you need?

Note though that from the high level you probably want to stick to HAL as defined, ie. no random parameters should be appended to the request, they need to be "declared" by the API.

@dblock dblock closed this Mar 13, 2016
@joshco
Copy link
Author

joshco commented Feb 6, 2017

Strangely, upgrading from Ruby 2.0.0 to 2.3.1 is breaking my add_query_params.
I know this pull request isn't an approved way to do things, so I'm hosted by my own petard.

Any kind folk have any idea why I'm getting this error below?

Before the call in the error, I'm setting query params via:

   resource_link.query_params={

          starting_after: horizon[:backward],
          starting_before: horizon[:forward],
          expand: 'codes'

      }

Then calling

 1) sync events
     Failure/Error: resources=resource_link['osdi:events']

     NoMethodError:
       undefined method `ascii_only?' for []:Array
     # /home/vagrant/.rvm/gems/ruby-2.3.1/bundler/gems/hyperclient-c6ee7f16a875/lib/hyperclient/link.rb:175:in `add_query_params'
     # /home/vagrant/.rvm/gems/ruby-2.3.1/bundler/gems/hyperclient-c6ee7f16a875/lib/hyperclient/link.rb:182:in `http_method'
     # /home/vagrant/.rvm/gems/ruby-2.3.1/bundler/gems/hyperclient-c6ee7f16a875/lib/hyperclient/link.rb:92:in `_get'
     # /home/vagrant/.rvm/gems/ruby-2.3.1/bundler/gems/hyperclient-c6ee7f16a875/lib/hyperclient/link.rb:87:in `_resource'
     # /home/vagrant/.rvm/gems/ruby-2.3.1/bundler/gems/hyperclient-c6ee7f16a875/lib/hyperclient/link.rb:131:in `method_missing'
     # ./app/services/osdi_sync.rb:259:in `sync_actions'

The error is with this code in the pull request:

https://github.com/codegram/hyperclient/pull/99/files#diff-6693c41efdfa7bd40a113952ff75dd45R174

query_array=URI.decode_www_form(uri.query || []).concat(new_query_array) # EXCEPTION HERE
def add_query_params(old_url, new_params)
     new_query_array= new_params.keys.map { |qi| [qi,new_params[qi]]}
     uri=URI(old_url)
     query_array=URI.decode_www_form(uri.query || []).concat(new_query_array) # EXCEPTION HERE
       uri.query=URI.encode_www_form(query_array)
     return uri.to_s
  end

@dblock
Copy link
Collaborator

dblock commented Feb 6, 2017

Is uri.query nil? It's a string, so I bet you that it's a bug and should be URI.decode_www_form(uri.query || '') or URI.decode_www_form(uri.query) || [].

Try to write a test for this?

@joshco
Copy link
Author

joshco commented Feb 13, 2017

That fixed it.

Try to write a test for this?
This code is in a rejected pull request, so I didn't but.
But I probably should have

@dblock
Copy link
Collaborator

dblock commented Feb 13, 2017

I re-read this, it wasn't clear to me that this wasn't in Hyperclient shipped code, my bad (too much time away from this codebase).

Have you looked at #92 @joshco? Maybe we can move that forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants