Draft: GraphQL: Return nil instead of GraphQL::CoercionError for TimeType
What does this MR do and why?
The graphql
gem returns nil
when attempting to coerce_input
with an invalid value, such as in https://github.com/rmosolgo/graphql-ruby/blob/c44aee192734a3b762d6030ed4a1cb352724ffc9/lib/graphql/types/iso_8601_date_time.rb#L53
# @param str_value [String]
# @return [Time]
def self.coerce_input(str_value, _ctx)
Time.iso8601(str_value)
rescue ArgumentError, TypeError
begin
Date.iso8601(str_value).to_time
rescue ArgumentError, TypeError
# Invalid input
nil
end
end
end
Our types should behave consistently with the gem's types. This also fixes a problem during the conversion in [graphql] Convert to using the new query interp... (!27536 - merged)
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %14.7
added GraphQL backend devopsplan sectiondev typemaintenance labels
assigned to @digitalmoksha
1 Warning c7c072ad: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Amparo Luna ( @a_luna
) (UTC-5, 1 hour ahead of@digitalmoksha
)Bob Van Landuyt ( @reprazent
) (UTC+1, 7 hours ahead of@digitalmoksha
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangermentioned in merge request !27536 (merged)
added workflowin review label
@alexkalderimis would you mind doing the first review on this?
requested review from @alexkalderimis
16 16 expect(described_class.coerce_isolated_input(iso)).to eq(time) 17 17 end 18 18 19 it 'rejects invalid input' do 20 expect { described_class.coerce_isolated_input('not valid') } 21 .to raise_error(GraphQL::CoercionError) 19 it 'returns nil for invalid input' do 20 expect(described_class.coerce_isolated_input('not valid')).to be_nil 22 21 end 23 22 24 it 'rejects nil' do 25 expect { described_class.coerce_isolated_input(nil) } 26 .to raise_error(GraphQL::CoercionError) 23 it 'returns nil for nil input' do 24 expect(described_class.coerce_isolated_input(nil)).to be_nil I think we need some full execution specs here so we know what the semantics are going to be. Something like:
let(:appointment) do time_t = described_class type_factory do field :start, null: false, type: time_t field :end, null: false, type: time_t end end let(:type) do time_t = described_class appointment_t = appointment type_factory do field :busy, null: false, type: GraphQL::Types::Boolean do argument :at, type: time_t end field :agenda, null: false, type: [appointment_t] def busy(at:) at.weekday? end def agenda [ { start: Time.now + 1.hours, end: Time.now + 2.hours } ] end end end it "can pass and receive good times" do str = <<<~GQL query { busy(at: "2021-01-01:01:01Z") agenda { start, end } } GQL r = execute_query(type, graphql: str) expect(r.to_h).to include { 'data' => { 'busy' => true 'agenda' => contain_exactly( { start: some_start, end: some_end } ) } } end it "can pass bad times and get an error in return" do str = <<<~GQL query { busy(at: "this is clearly not a valid time") } GQL r = execute_query(type, graphql: str) expect(r.to_h).to include { 'errors' => [ ??? ] } end
I think full execution specs are the only way to know that our implementation is behaving correctly in the presence of the framework.
Thanks for doing this. I think to make this change we need some more extensive tests so we can refactor with confidence.
Back to you @digitalmoksha
requested review from @digitalmoksha and removed review request for @alexkalderimis
removed review request for @digitalmoksha
Setting label groupproject management based on
@digitalmoksha
's group.added groupproject management label
changed milestone to %14.8
added missed:14.7 label
added 3135 commits
-
ab9feafd...5c90a359 - 3133 commits from branch
master
- a0675335 - Return nil instead of GraphQL::CoercionError
- 4858ca22 - wip
-
ab9feafd...5c90a359 - 3133 commits from branch
marked this merge request as draft from 4858ca22
added 1302 commits
-
4858ca22...f44a5cf3 - 1300 commits from branch
master
- c4a7202a - Return nil instead of GraphQL::CoercionError
- c7c072ad - wip
-
4858ca22...f44a5cf3 - 1300 commits from branch