Refactor parameter sorting

[wishdev]

Description

This is a quasi-second attempt at #469 which respects the comment in it #469 (comment)

As opposed to adding any new options - this simply moves the sort call into a method which can be overridden as opposed to requiring the "cut and paste" of the standard modules into a custom module.

So with this patch one can accomplish no param sorting by simply adding

module Faraday
  module NestedParamsEncoder
    def self.sort_params(_); end
  end

  module FlatParamsEncoder
    def self.sort_params(_); end
  end
end

To their code, because sort_params can be overridden.

Again, I completely respect the desire to keep it clean - I'm just wondering if this slight refactor would be ok.

[iMacTia]

Hey @wishdev, appreciate you trying to fit this in following the first feedback in the other PR.
I think I can relate to Mislav's comment, it's really easy to add small tweaks into your gem, until over time the number of tweaks grows beyond control.

The biggest issue I have with your change is that you're changing the default behaviour of the gem.
This might fit your requirements, but may potentially break other existing implementations.
For that reason, we would need to change your PR so that sort_params actually does not do any sorting, and you'd have the opportunity to override it in your project, in order to make it sort them for real.
As you can imagine this would be quite dumb (a method called sort_params that doesn't sort 😄).
Moreover, you'd still need to override this new method in your project.

Luckily, Ruby is an OOP language and as such it should already give you all the tools you need, so let me show you what I think it's the best solution in your case and, most probably, what Mislav was also referring to in his comment:

class MyEncoder < Faraday::NestedParamsEncoder # or FlatParamsEncoder
  self.encode(params)
    super
    params.sort!
  end
end

# Then later when you initialise your Faraday Connection
conn = Faraday.new(url, request: { params_encoder: MyEncoder })

There you go, no change necessary in the Faraday gem itself, and just 6 lines of code in your project.
I haven't tested the code above, but I'm sure that will give you the idea of what I mean.

I'm closing this PR in the hope that the above helps you with what you're trying to do, but if you have any questions then feel free to follow-up.

[wishdev]

I apologize but you misread the code. The current implementation sorts. I do not want a sort and therefore seek the change to allow the sort_params addition to be a passthrough.

There is no change at all to the default and current implemenation. This is simple a decomposition of the encode method to allow for a more granular override capability.

[iMacTia]

I'm sorry @wishdev, I most definitely did!

To my surprise, we currently sort parameters indeed. I was unaware of that bit but it seems like it has been like that over the past 8 years 😮.

Based on everything I said above, your approach makes sense to me and it will allow devs to override that opinionated behaviour when necessary

[iMacTia]

I was curious to see if tests were failing, but there seems to be an issue with one of the dependencies, I'll try to have a look when I get a chance

[iMacTia]

@wishdev Tests are green now, it seems like it was a temporary issue with GitHub Actions.
I'm OK with this PR now, but I wanted to propose a "compromise" solution as well before we proceed.
I've reviewed #469 and I actually don't think this solution is much better than adding a new option, BUT sticking that new option into the Faraday connection caused changes in multiple files, which I don't personally like.

I'd be happy to review and approve an alternative solution where instead of introducing a new method, we introduce a class-level option in both the encoders to enable or disable the sorting of parameters (and of course it would default to being enabled for backwards-compatibility).
It would work like this:

# The same can be done for Faraday::NestedParamsEncoder
Faraday::FlatParamsEncoder.sort_params = false # default value is true

The change would be limited to the encoders (thus not touching the connection) and changing the behaviour would be as easy as to set the new option to false, with no need to override methods.
What do you think of this revised, middle-way proposal @wishdev?

[wishdev]

# The same can be done for Faraday::NestedParamsEncoder
Faraday::FlatParamsEncoder.sort_params = false # default value is true

The change would be limited to the encoders (thus not touching the connection) and changing the behaviour would be as easy as to set the new option to false, with no need to override methods.
What do you think of this revised, middle-way proposal @wishdev?

I believe it is an excellent idea @iMacTia. I started with the add nothing concept and I like middle grounds. I've pushed the change as you requested along with specs.

[iMacTia]

Thanks @wishdev, I really appreciate you trying to satisfy Mislav's initial comment.
I'm happy with this new implementation, thanks for your patience and contribution!
I'm checking why Rubocop is complaining, most probably not your PR's fault so I'll investigate it separately 👍

[iMacTia]

@wishdev I did some digging about the Rubocop issue and I found this comment.
The reason seems to be that your lines FlatParamsEncoder.sort_params = true and NestedParamsEncoder.sort_params = true are inside the module Faraday block, causing Rubocop not to consider it a namespace (as it should be).

May I ask you one last change to your PR by moving those lines outside the module Faraday block?
That should fix the Rubocop issues 👍

[iMacTia]

See comment above

[wishdev]

My apologies for not understanding what the rubocop issue was concerning.

I believe this should move things along.

[iMacTia]

No worries! It took me some time as well to dig it out, thanks for fixing it so quickly 🙌
Code looks great, tests are all green and rubocop is happy 🎉
LGTM

[sergeymavrin]

Sorry for my nubian question, but how I can use option sort_params = false with such connection description?

@connection = Faraday.new(url: cfg['http_url']) do |req|
  req.request :url_encoded
  req.headers['Content-Type'] = 'application/x-www-form-urlencoded'
  req.adapter Faraday.default_adapter
end

[iMacTia]

@sergeymavrin you simply need to set the sort_params property in the params encoder you're using.
If you're unsure which one you're using, it's probably the default one (NestedParamsEncoder), or you could use the Faraday::Utils helper to find out:

# If you know which encored you're using
Faraday::NestedParamsEncoder.sort_params = false
# or
Faraday::FlatParamsEncoder.sort_params = false

# If you want to use the Faraday::Utils helper
Faraday::Utils.default_params_encoder.sort_params = false

[sergeymavrin]

@iMacTia Thank You

[davidbasalla]

I've run into a situation where I need to set sort_params = false but I've got multiple Faraday clients in my app (and I only want to disable sorting for one specific client). What's the best way of disabling sorting for a specific Faraday client? Iiuc @sort_params is a class variable (?) on the FlatParamsEncoder module, so running Faraday::NestedParamsEncoder.sort_params = false would change it for all Faraday clients in my app? Can I subclass/include FlatParamsEncoder somehow and then set @sort_params to false?

[iMacTia]

@davidbasalla mmmh I wish there was a nicer way to do this, but you're right, this is a class variable setting right now with no way to set it. The whole API of the param encoders are very "classy" right now (mostly class methods), so changing this on a per-client basis is not straightforward.

A possible way around this, as you already pointed out, is to subclass/include the ParamsEncoder you wish to use, and then pass it to a specific client instance as shown here. However this solution requires you to have access to the initilization configuration of your connection, which may not be the case for you.

If that's still a problem, then things might get messy. I'd be happy to assist further but I'd suggest to move this into a separate issue so we can brainstorm the best way forward 👍

[davidbasalla]

thanks for your reply @iMacTia. For the moment I've resorted to cloning the FlatParamsEncoder module on start up of my app and changing sort_params on the clone (which looks a bit non-standard but seems to work)

module Faraday
  UnsortedFlatParamsEncoder = FlatParamsEncoder.clone
  UnsortedFlatParamsEncoder.sort_params = false
end

It seems to solve the problem for me. I'll create a separate issue if I run into any more problems with it 👍