Fail to mount an API with default values on mutually_exclusive params

[gencer]

Definition:

params do
   optional :folder, type: String, default: nil
   optional :tag, type: Array, default: nil
   exactly_one_of :folder, :tag
end

header:

Content-Type: application/json;

request body:

{
    "tag": [1,2,3]
}

or

{
    "folder": "/hello"
}

got:

{
    "error": "folder, tag are mutually exclusive"
}

[rnubel]

Does removing default: nil for both params change the behavior?

[gencer]

Definitely, did! Also, error message now shows the correct message. (not for mutually)

Instead of default value, I set allow_blank and/or regexp for better data integrity.

[dblock]

Still a bug, right?

[gencer]

Yes. Default value can be useful. Exactly one of means one of them will be empty or have a default value I gave.

Update: Oh, yes, also giving a default value turns them to mutually exists parameters which is a bug.

[dblock]

Would appreciate a pull request even if it's just failing specs.

[gencer]

Before I send a PR, lets make sure my steps are correct.

After I review the code, I found that, In fact :default should be dropped. In exactly_one_of we should not use default. Because, if we do that then we cannot know which parameter given by user or by default. Due to this grape simply tell us that they are mutually exclusive, in other mean it is limited.

So, I no longer thinks this is a bug. This is an expected behavior.

I think clearly stating this on documentation will be enough.

[dblock]

Aha. If both values have a default it already violates exactly_one_of, but maybe if one of them has a default and you don't pass anything, then it should work?

[gencer]

Yes. To be clear;

Definition:

params do
   optional :folder, type: String, default: nil
   optional :tag, type: Array, default: nil
   exactly_one_of :folder, :tag
end

Request:

{
    "folder": "/hello"
}

It will fail. Why? Because I only give :folder but I also give default: nil for :tag in definition too. :tag should NOT exists in params as a key. It should not have a value like nil or empty string. It should not exists in the first place.

I believe current behavior is the more efficient and correct behavior. We just need to tell that "do not assign even nil values" to any exactly_one_of definitions.

At first I thought giving a default value should be okay. But we should not. Exactly one parameter should be exists in params bag. So we can check which key is exists not by nil or emptyness.

To prevent empty and nil values, we should use proc or regexp.

[dblock]

Correct. A default of nil is still a default. Maybe rewrite the title of this issue to say something like the above scenario should not even load and produce an error?

[gencer]

This has been clarified by adding a note. #1720.

[ghiculescu]

I think https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#nil-values-for-structures is causing new problems similar to this issue.

params do
   optional :folder, type: Array[String], default: []
   optional :tag, type: Array[String], default: []
   mutually_exclusive :folder, :tag
end

This will error even if neither param is provided.

[dblock]

If neither param is provided then both get the default value and those are mutually exclusive, which errors by design. LGTM.

[ghiculescu]

The problem for me is this is a regression - it wasn't an issue before https://github.com/ruby-grape/grape/blob/master/UPGRADING.md#nil-values-for-structures, because we didn't need to provide a default. Which meant the mutually_exclusive validation would only error if both props were actually provided.

Now, requests that previously wouldn't fail will now fail. The alternative is to not use default: [] but then you get this regression instead:

  # 1.3.1 = []
  # 1.3.2 = nil

[dblock]

I understand this is not the way it behaved before, but I would say that this was a bug, fixed since. We probably didn't even have specs for this, hence a minor version bump and a regression. I am not saying we did it on purpose, but we're trying to avoid similar problems moving forward.

If you have a mutually exclusive set of values that are both set when none are provided by the caller they are no longer mutually exclusive, which violates the principle of least surprise. Saying "they are mutually exclusive to be supplied by the user" is interpreting mutually_exclusive conveniently.

I think the workaround is straightforward:

params do
   optional :folder, type: Array[String]
   optional :tag, type: Array[String]
   mutually_exclusive :folder, :tag
end
get do
  folder_or_tag = params[:folder] || params[:tag] || []
  ...
end

Thoughts?

[dblock]

Now that we have more attention on this issue, I think we should close this. IMHO it behaves as intended.

Taking this back. An API with mutually exclusive and default values should not mount. This is the feature request.

[ghiculescu]

Okay. I’m happy with that.

On Tuesday, May 26, 2020, Daniel Doubrovkine (dB.) @dblockdotorg < notifications@github.com> wrote:

Now that we have more attention on this issue, I think we should close this. IMHO it behaves as intended.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ruby-grape/grape/issues/1717#issuecomment-634370708, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD4PDJ5BG2WPT3UL3XI5UDRTRUURANCNFSM4EINFIKA .