Wierd behavior with optional array

[lasseebert]

Hi. First of all, thanks for a great framework.

I think I have found a bug:

If I have an optional array of hashes and one of the hash values has a default, the resulting param is weird when not provided.

Example:

params do
  optional :some_array, type: Array do
    optional :foo, default: "bar"
  end
end

When not providing :some_array, I would expect params[:some_array] to be nil, but it is {foo: "bar"} (which is not even an array).

In this pull request I have med a spec that shows this.

Are my expectations right?

[dm1try]

@lasseebert , thanks for opened issue and spec. Your expectations are right. Recently I updated default validator to fix another kind of problem with it and this behavior is a bug :( seems like today I'll go deeper and do refactor some part of DefaultValidator.

[dm1try]

BTW, @lasseebert I'd like to hear your opinion about this spec ?)

[lasseebert]

A tough one.

I think it makes most sense to not use inner default values when the hash itself is not represented in the request.

Consider this:

optional :my_hash, type: Hash do
  requires :foo
  optional :bar, default: "baz"
end

A required param inside an optional parent should only be required if the parent is present. But if the optional :bar param is created automatically without the parent being part of the request, then :foo param would be required. Does not make sense.

A more sane way would be to only consider inner params if the parent hash is represented in the request.

This was just my humble oppinion. I have only worked with Grape for a couple of months, and I'm not sure if my proposals will have bad side effects.

[dm1try]

@lasseebert , good opinion! Honestly, I have same feelings about this problem. Thx)

[lasseebert]

Thanks. Let me know if you need me to do anything ;)

[dm1try]

@lasseebert , sure :)

@dblock , see comments above, what do you think. Can we revert changes provided by #538, #537 in this version?

/cc @dsager

[dblock]

@dm1try - I am not sure I completely understand this. Want to put up a PR and I'll read things carefully?

[dsager]

Hi everyone,
seems like we got slightly different expectations of what a default value does :)

When I need to rely on the presence of a certain parameter, I specify a default value. And when I need a hash to have certain key-value pairs, I also specify default values.

So the motivation for committing the above mentioned spec (46c2737) was the need for a hash parameter whose values are always present. The most frequent use case is a paging parameter hash:

optional :page, type: Hash do
    optional :number, type: Integer, desc: 'Page number', default: 1
    optional :size, type: Integer, desc: 'Page size', default: 10
end

With this code I can use params[:page][:number] and params[:page][:size] without worrying about them not being present. The way it works in @lasseebert's example is not what I had in mind though :)

Looking at the code now it seems like the handling of groups changed anyway? In version 0.6.x :page was not treated as a parameter of it's own.

[lasseebert]

Hi @dsager

Thanks for taking the time for this :)

I agree that it's not abvious what would be a the desired expectation for inner default values when dealing with Hash values. My original concern was when dealing with arrays:

optional :some_array, type: Array do
  optional :key_1, default: "foo"
  requires :key_2
end

When nothing about :some_array is submitted in the request, it does not make sense to create e.g. a single item in the array.

The behavior now is that the array param is set to a hash, which is a little weird:

post '/something' do
  params[:some_array] # will result in { key_1: "foo" } when :some_array is not provided
end

[dsager]

Hi @lasseebert,
I see your point and agree that it doesn't make much sense in your example. So we got two different behaviours for defaults of sub-parameters:

• always provide a value
• only provide values when there's at least one value for another sub-param present

I think both behaviours make sense in respective use cases. So I'd love to see both supported. Question is how? Maybe a default option for the group itself:

optional :foo, type: Hash, default: { bar: 'foo-bar' } do
    optional :bar, type: String
end

Or have an additional option to explicitly enforce default-only groups:

optional :foo, type: Hash, always: true do
    optional :bar, type: String, default: 'foo-bar'
end

What do you think about this, @dm1try & @dblock?

Thanks for discussing this!

[lasseebert]

Hi @dsager

I really like the idea of always: true for parameters of type: Hash. It makes much sense IMO ;)

For parameters of type: Array I guess the always option should indicate if the parameter is nil or [] when not specified?

[dsager]

Hi @lasseebert,
tbh I was expecting it to work the same way for type: Array and type: Hash ;)
As soon as you specify always: true it doesn't make sense to have a required parameter without a default...

[lasseebert]

Hi @dsager

Just to make sure I understand you right:

When declaring

optional :foo, type: Array, always: true do
  optional :bar, default: "bar"
end

What would params[:foo] be when that parameter is not specified in the request?

[dsager]

Hey @lasseebert,
I'd say params[:foo] should be "bar", just because you say explicitly that you want this to happen. But this is just my opinion :D

[lasseebert]

But params[:foo] is declared to be an array. That can't possibly also be a string? ;)

[dsager]

Ah sorry, i got confused. I meant that the value for :bar should be "bar". So params[:foo] should be [{ bar: "bar" }]. But tbh I never used the array type before, so maybe it's better to ignore my opinion on this :p

[lasseebert]

@dsager

Ok, I understand now ;)
Please let me know if I can be of any help implementing some of this.

[dsager]

@lasseebert, note that I'm just user/contributor and not a collaborator, so the final word on this should come from @dm1try or @dblock ...

:)

[yaneq]

always: true

It might be a good idea to make the parameter more descriptive, e.g. add_defaults_if_missing: true or similar.

[dblock]

Oh wow, this is one is interesting.

I'll start by the tail-end. Saying optional ... always: true sounds super weird from the API developer perspective, whatever you call it. We should pick an opinion about how defaults are handled and potentially (not ideally) be OK that someone doesn't quite get their custom behavior.

Here's what I would propose:

optional :foo, type: Array do
  optional :bar, default: "bar"
end

In this example foo is optional and has no default value, so it should be nil. We shouldn't even be looking further. So I think I disagree with @dsager on this one.

However,

optional :foo, type: Array, default: [] do
  optional :bar, default: "bar"
end

When this parameter is not present, it defaults to []. And we should stop looking further, again.

Lets look at a hash:

optional :foo, type: Hash, default: {} do
  optional :bar, default: "bar"
end

This is the same thing, it defaults to {} when unspecified. However, if it is specified, then we should check the value of the optional bar. This is because if you want to have a default value for bar when the whole hash is not specified you should be able to write:

optional :foo, type: Hash, default: { bar: 'baz' } do
  optional :bar, default: "bar"
end

I think this would be explicit and consistent for all types. The way I think about it is "i'm checking a parameter, it's not there, apply its default and stop at that".

What do you think?

[dblock]

Btw, this bug describes exactly what I am thinking I think :)

[lasseebert]

Very nice and concise, @dblock.

I really like this!

[yaneq]

To bring back the paging example:

optional :page, type: Hash, default: { number: 1, size: 10 } do
    requires :number, type: Integer, desc: 'Page number'
    optional :size, type: Integer, desc: 'Page size', default: 10
end

While i agree that its intuitive, it will add some duplication in some situations.

Alternatively

optional :page, type: Hash, add_defaults_if_missing: true do
    requires :number, type: Integer, desc: 'Page number', default: 1
    optional :size, type: Integer, desc: 'Page size', default: 10
end

would not have that issue, but requires with default does not make a lot of sense either. So i guess the first solution is probably the best trade-off.

[dblock]

@yaneq We should solve that problem separately. Ambiguity is certainly worse than duplication.

What if:

optional :page, type: Hash, default: { number: 1, size: 10 } do
    requires :number, type: Integer, desc: 'Page number'
    optional :size, type: Integer, desc: 'Page size', 
      default: -> { default[:size] } # or even just default[:size] if we can
end

Otherwise I would declare DEFAULT_PAGE_SIZE = 10, just to make a statement that these should be the same.

[dsager]

Interesting indeed :)
Also a possibility would be to merge provided values into the default and don't have defaults for the sub-params.

Assuming:

optional :page, type: Hash, default: { number: 1, size: 10 } do
    optional :number, type: Integer
    optional :size, type: Integer
end

Expecting:

• &page[number]=2&page[size]=5 results in

{ page: { number: 2, size: 5 } }

• &page[number]=2 results in

{ page: { number: 2, size: 10 } }

• &page[size]=5 results in

{ page: { number: 1, size: 5 } }

• nothing results in

{ page: { number: 1, size: 10 } }

[dm1try]

Great job, guys! Who want to help with implementation? :)

[lasseebert]

@dm1try I have not yet implemented anything in Grape, but let me know if I can help with code review or implementation ;)

[dsager]

did we agree what solution to implement yet? :)

[dblock]

I can play the role of the benevolent dictator. I want the solution that I described above. No hard feelings :)

[dm1try]

Challenge accepted! Sorry for delay 😬

[dblock]

Bump.

[dm1try]

@dblock , sorry. It's a little hard to implement the solution described by you for now.
I did a little bit progress. See, dm1try@d29c822
But I wanna refactor some validation logic to be able implement

The way I think about it is "i'm checking a parameter, it's not there, apply its default and stop at that".

Need more time or some help/suggestions) Thanks.

[dblock]

@dm1try Any suggestions of what to do with this?

[dm1try]

@dblock Oh, it is hard for me at the moment :) I didn't implemented the solution described above.

[dblock]

Fixed in #936, thanks @dm1try!