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

Support duplicate form names in multipart forms #32

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ixti
Copy link
Member

@ixti ixti commented May 27, 2021

Resolves: httprb/http#663

@ixti
Copy link
Member Author

ixti commented May 27, 2021

This change is Reviewable

@mathisto mathisto self-assigned this May 28, 2021
@mathisto mathisto force-pushed the multi-part-form-data-pairs branch from 571e0c5 to 2631b36 Compare May 28, 2021 04:04
@mathisto
Copy link
Collaborator

mathisto commented May 28, 2021

@ixti I added a line to your spec for multipart 'pairs'. I think that a boundary is required between each 'body part' as per RFC 1341 7.2. You should double check me though, I don't wade in these water's every day.

I altered the coerce method to take an Array or Hash. We had talked about a more generic Enumerator but I think it might be better to restrict rather than abstract in this case.

Additionally, I added a spec helper config that was really helpful for figuring out what was failing. I suspect the config is not everyone's cup of tea since the default max_formatted_output_length is 200. Let me know if you want it ganked or if it can/should stay.

Let me know what you think. Would love to talk shop.

@mathisto mathisto force-pushed the multi-part-form-data-pairs branch from 14c6985 to efb5549 Compare May 28, 2021 04:41
@mathisto mathisto changed the title WIP: Support duplicate form names in multipart forms Support duplicate form names in multipart forms May 28, 2021
@mathisto mathisto marked this pull request as ready for review May 28, 2021 04:54
@ixti
Copy link
Member Author

ixti commented May 28, 2021

@ixti I added a line to your spec for multipart 'pairs'. I think that a boundary is required between each 'body part' as per RFC 1341 7.2. You should double check me though, I don't wade in these water's every day.

Yeah, thank you. It was unintentional typo from my end.

I altered the coerce method to take an Array or Hash. We had talked about a more generic Enumerator but I think it might be better to restrict rather than abstract in this case.

After some consideration, I think I'm actually more in favour of more flexible handling. What I mean is, we allow this:

{ :foo => [1, 2, 3] }

That is supported now, and will create 3 parts with same name, but different values (1, 2, and 3), so we should support at least:

[:foo, [1, 2, 3]]

Which we do with this RP currently. But why not allow any Enumerable? For example:

some_array.lazy.select(&some_filter)

Right now we will convert the above to Hash and if it yields duplicate keys - it will be inconsistent with Array.

Additionally, I added a spec helper config that was really helpful for figuring out what was failing. I suspect the config is not everyone's cup of tea since the default max_formatted_output_length is 200. Let me know if you want it ganked or if it can/should stay.

I think I'm more against this one than agree with. :D It can be easily done local-only adding:

# file: .rspec-local
--require ~/.my-rspec-config.rb
# file: ~/.my-rspec-config.rb

RSpec.configure do |config|
  # overwrite project's config as you feel it :D
end

@ixti
Copy link
Member Author

ixti commented May 29, 2021

@mathisto I have rewrote tests a bit (and added similar test to urlencoded one too).

@ixti
Copy link
Member Author

ixti commented May 29, 2021

After giving it a second thought, Rack and Addressable support only nil, Array, and Hash. So, I guess I'm fine with proposed solution.

@ixti
Copy link
Member Author

ixti commented May 29, 2021

@mathisto can you, please, make sure that both urlencoded and multipart encoders behave consistently?

lib/http/form_data/multipart.rb Outdated Show resolved Hide resolved
Co-authored-by: Alexey Zapparov <alexey@zapparov.com>
@ixti
Copy link
Member Author

ixti commented May 29, 2021

The more I think the more I believe that there's no point in limiting to Array/Hash. I mean we really only care that it's an Enumerable that yields at one or two arguments, and we don't care about the rest:

[[1], [2, 3], [4, 5, 6]].map { |k, v| "#{k}=#{v}" }.join("&")
# => "1=&2=3&4=5"

{ 1 => nil, 2 => 3 }.map { |k, v| "#{k}=#{v}" }.join("&")
# => "1=&2=3"

Enumerator
  .new { |y| y << [1] << [2, 3] << [4, 5, 6] }
  .map { |k, v| "#{k}=#{v}" }
  .join("&")
# => "1=&2=3&4=5"

So, I'm not really sure why we treat Hash and Array specially. I agree, that Enumerator or any kind of Enumerable as an input value makes not much sense, and probably will never be used, but I don't really see why would treating Hash and Array only is better than being less strict. :))

@ixti
Copy link
Member Author

ixti commented May 30, 2021

In fact I think I would accept any input that responds to :each, but in most cases those that implement #each, also include Enumerable, thus, I believe Enumerable is the best candidate :D

@mathisto
Copy link
Collaborator

mathisto commented Jun 1, 2021

Ok I like it. Especially dropping the coerce. I'll knock this out tonight after the kids are in bed.

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.

Specific ordering of duplicate key names for multipart data.
2 participants