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

Allow "null" to be passed in filters #142

Open
richmolj opened this issue May 15, 2019 · 5 comments
Open

Allow "null" to be passed in filters #142

richmolj opened this issue May 15, 2019 · 5 comments

Comments

@richmolj
Copy link
Contributor

@richmolj richmolj commented May 15, 2019

And convert to Ruby's nil

@richmolj
Copy link
Contributor Author

@richmolj richmolj commented May 15, 2019

Did some initial work here: https://github.com/richmolj/graphiti/tree/filter_nil

Though the work is pretty straightforward, there is a purpose to disallowing nils. Passing a nil can sometimes cause slow/expensive database queries, and it should probably be disabled by default. This means we probably need 2 options

  • filter :foo, allow_nil: true
  • self.filters_accept_nil_by_default = true (defaults to false`)

@wadetandy sound about right?

@wadetandy
Copy link
Contributor

@wadetandy wadetandy commented May 15, 2019

Makes sense and I agree that we should not change the defaults. If we're doing some nice things on behalf of nil, it might be a good idea to do the same for javascript's undefined, though I can also see this being a more common thing that is actual a desired filter. Maybe have a configurable list of "strings we coerce to nil" with sensible defaults?

@zeisler
Copy link
Contributor

@zeisler zeisler commented May 15, 2019

I would recommend following the JSON spec that undefined is an invalid conversion to Ruby.

> JSON.parse("undefined")
  #=> JSON::ParserError (416: unexpected token at 'undefined')
> JSON.parse("null")
  #=> nil
@richmolj
Copy link
Contributor Author

@richmolj richmolj commented May 15, 2019

Yeah, I agree with @zeisler - also makes sense because in Spraypaint, I would drop undefined and not send as part of the request, but null should turn into the string "null".

Maybe we'll move to a configurable whitelist at some point, but that seems good for now.

@wadetandy
Copy link
Contributor

@wadetandy wadetandy commented May 15, 2019

In general I would Be in favor of either allowing nil and undefined OR allowing neither, but I’m not going to get too worked up about any outcome here.

zeisler added a commit to zeisler/graphiti that referenced this issue Jun 11, 2019
Filters have new options `allow_nil: true`
Option can be set at the resource level `Resource.filters_accept_nil_by_default = true`
By default this is set to false.

graphiti-api#142
zeisler added a commit to zeisler/graphiti that referenced this issue Jun 12, 2019
Filters have new options `allow_nil: true`
Option can be set at the resource level `Resource.filters_accept_nil_by_default = true`
By default this is set to false.

graphiti-api#142
zeisler added a commit to zeisler/graphiti that referenced this issue Jun 12, 2019
Filters have new options `allow_nil: true`
Option can be set at the resource level `Resource.filters_accept_nil_by_default = true`
By default this is set to false.

graphiti-api#142
zeisler added a commit to zeisler/graphiti that referenced this issue Jun 12, 2019
Filters have new options `allow_nil: true`
Option can be set at the resource level `Resource.filters_accept_nil_by_default = true`
By default this is set to false.

graphiti-api#142
zeisler added a commit to zeisler/graphiti that referenced this issue Jun 13, 2019
Filters have new options `allow_nil: true`
Option can be set at the resource level `Resource.filters_accept_nil_by_default = true`
By default this is set to false.

graphiti-api#142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.