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

Strict validation on ISO8601Date #4228

Closed
nbcraft opened this issue Oct 13, 2022 · 2 comments
Closed

Strict validation on ISO8601Date #4228

nbcraft opened this issue Oct 13, 2022 · 2 comments

Comments

@nbcraft
Copy link

nbcraft commented Oct 13, 2022

Note: I can implement any solution that we would agree upon (if any), I just want to use this issue to discuss first.

Is your feature request related to a problem? Please describe.

We ran into an issue where our frontend's DatePicker component would attach the current user browser time to the selected date, and then convert it to a UTC DateTime (ie 2022-10-02T23:08:43.567Z) before sending it to our ISO8601Date graphql argument.

But the conversion to UTC means that it might show a different day than the one picked by the user, because of their timezone.

But the gem code parsing the value sent will tolerate having time information in there, and just truncate to the date information:

Describe the solution you'd like

I would like a validation that ensure we're not passing time information to this argument type, and rejects when it is the case.
Because if you need time information, then ISO8601DateTime type exists.

What I did in my app is re-open the class and change the line above to Date.iso8601(value, limit: 10) which will raise if the string is longer than 10 characters. We might actually want 11 to allow for longer date formats, like mentioned in the ruby doc:

[Date.iso8601(string='-4712-01-01'[, start=Date::ITALY], limit: 128) → date](https://rubyapi.org/3.1/o/date#method-c-iso8601)

Date.iso8601('2001-02-03')        #=> #<Date: 2001-02-03 ...>
Date.iso8601('20010203')          #=> #<Date: 2001-02-03 ...>
Date.iso8601('2001-W05-6')        #=> #<Date: 2001-02-03 ...>

Not sure if this should be default or just an option to have a strict validation.

Describe alternatives you've considered

Alternatively, if we're okay to enforce the YYYY-MM-DD format for ISO8601Date types, then we could use Date.strptime:
Date.strptime('2001-02-03', '%Y-%m-%d') #=> #<Date: 2001-02-03 ...>

Additional context

@rmosolgo
Copy link
Owner

Hi! Sorry for the trouble you encountered with this.

Unfortunately, I consider my hands tied with this built-in type: if we were to make it more strict, it'd be a breaking change to everyone who's using it. (It's arguably a better behavior, but regardless, I don't want to start breaking things that aren't already broken for people!)

My suggestion would be to implement a custom scalar based on your situation, for example:

# A custom date type with more strict input parsing
class Types::ISO8601Date < GraphQL::Types::ISO8601Date
  def self.coerce_input(val, ctx)
    Date.iso8601(val, limit: 11)
  end 
end 

Please let me know if you run into any trouble with an approach like that!

@nbcraft
Copy link
Author

nbcraft commented Oct 26, 2022

Hey @rmosolgo, thanks for your reply.

Yeah I actually had implemented that solution locally before choosing the re-open the class itself, to be sure we don't use the "wrong" type accidentally.

I also figured it would be a breaking change, a few notes about that:

  • That's what major versions are for, but I agree it's a bit aggressive.
  • Maybe we could have a type called ISO8601DateStrict or something?
  • Could it be possible to have the strict validation be activated via a param on a ISO8601Date argument?

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

No branches or pull requests

2 participants