Skip to content
Snippets Groups Projects

Draft: GraphQL: Return nil instead of GraphQL::CoercionError for TimeType

Closed Brett Walker requested to merge bw-time-type-coerce into master
1 unresolved thread

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.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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.

  • Please register or sign in to reply
  • 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

  • Alex Kalderimis requested review from @digitalmoksha and removed review request for @alexkalderimis

    requested review from @digitalmoksha and removed review request for @alexkalderimis

  • Brett Walker removed review request for @digitalmoksha

    removed review request for @digitalmoksha

  • Setting label groupproject management based on @digitalmoksha's group.

  • 🤖 GitLab Bot 🤖 changed milestone to %14.8

    changed milestone to %14.8

  • Brett Walker added 3135 commits

    added 3135 commits

    Compare with previous version

  • Brett Walker marked this merge request as draft from 4858ca22

    marked this merge request as draft from 4858ca22

  • Brett Walker added 1302 commits

    added 1302 commits

    Compare with previous version

  • I don't think I need this right now. Closing, and will re-open if necessary.

  • closed

  • Please register or sign in to reply
    Loading