Return ordered result hashes #5060

rmosolgo · 2024-08-12T20:23:09Z

The spec says that JSON key-value pairs should come back in the same order that they were requested in the query. I need an implementation that complies with the spec and adds as little overhead as possible (memory and runtime). Possibly, if it adds noticeable overhead, make it opt-in, since this is critical performance code

I need to somehow maintain the sequence of this result even when Dataloader or GraphQL-Batch write to it out of order (see #4252).

I think the selections_by_name hash(es) have the correct order of keys here:

graphql-ruby/lib/graphql/execution/interpreter/runtime.rb

Lines 120 to 192 in d9bd9cf

    
           def gather_selections(owner_object, owner_type, selections, selections_to_run = nil, selections_by_name = {}) 
        
             selections.each do |node| 
        
               # Skip gathering this if the directive says so 
        
               if !directives_include?(node, owner_object, owner_type) 
        
                 next 
        
               end 
        
               if node.is_a?(GraphQL::Language::Nodes::Field) 
        
                 response_key = node.alias || node.name 
        
                 selections = selections_by_name[response_key] 
        
                 # if there was already a selection of this field, 
        
                 # use an array to hold all selections, 
        
                 # otherwise, use the single node to represent the selection 
        
                 if selections 
        
                   # This field was already selected at least once, 
        
                   # add this node to the list of selections 
        
                   s = Array(selections) 
        
                   s << node 
        
                   selections_by_name[response_key] = s 
        
                 else 
        
                   # No selection was found for this field yet 
        
                   selections_by_name[response_key] = node 
        
                 end 
        
               else 
        
                 # This is an InlineFragment or a FragmentSpread 
        
                 if @runtime_directive_names.any? && node.directives.any? { |d| @runtime_directive_names.include?(d.name) } 
        
                   next_selections = {} 
        
                   next_selections[:graphql_directives] = node.directives 
        
                   if selections_to_run 
        
                     selections_to_run << next_selections 
        
                   else 
        
                     selections_to_run = [] 
        
                     selections_to_run << selections_by_name 
        
                     selections_to_run << next_selections 
        
                   end 
        
                 else 
        
                   next_selections = selections_by_name 
        
                 end 
        
                 case node 
        
                 when GraphQL::Language::Nodes::InlineFragment 
        
                   if node.type 
        
                     type_defn = query.types.type(node.type.name) 
        
                     if query.types.possible_types(type_defn).include?(owner_type) 
        
                       result = gather_selections(owner_object, owner_type, node.selections, selections_to_run, next_selections) 
        
                       if !result.equal?(next_selections) 
        
                         selections_to_run = result 
        
                       end 
        
                     end 
        
                   else 
        
                     # it's an untyped fragment, definitely continue 
        
                     result = gather_selections(owner_object, owner_type, node.selections, selections_to_run, next_selections) 
        
                     if !result.equal?(next_selections) 
        
                       selections_to_run = result 
        
                     end 
        
                   end 
        
                 when GraphQL::Language::Nodes::FragmentSpread 
        
                   fragment_def = query.fragments[node.name] 
        
                   type_defn = query.types.type(fragment_def.type.name) 
        
                   if query.types.possible_types(type_defn).include?(owner_type) 
        
                     result = gather_selections(owner_object, owner_type, fragment_def.selections, selections_to_run, next_selections) 
        
                     if !result.equal?(next_selections) 
        
                       selections_to_run = result 
        
                     end 
        
                   end 
        
                 else 
        
                   raise "Invariant: unexpected selection class: #{node.class}" 
        
                 end 
        
               end 
        
             end 
        
             selections_to_run || selections_by_name 
        
           end

I can't find any way to re-order a Ruby Hash. So if I'm going to use a Hash, need to write the keys in the right order the first time. (Technically, you can move a key to the back by doing h[k] = h.delete(k), but I don't think that's a promising approach.)

Another approach might be to use an Array to hold values and then build a Hash from there. For example, an array of [k1, v1, k2, v2, ...] could be used. One problem here is that it will require a second pass through the whole data structure. GraphQL-Ruby used to do this, but it was slow. I'd like to not make it do that again.

Maybe I could mitigate the downside of the second iteration by implementing Result#to_json to return a properly-ordered string. This would work out-of-the-box with Rails... but I know a lot of JSON serializers are really optimized. Could GraphQL-Ruby deliver good performance in that case? (And if I take that approach, I should implement .to_ordered_h and test it against .to_ordered_h.to_json with an optimized JSON serializer.)

graphql-js handles this by writing promises into the result hash. This is also how GraphQL-Ruby happens to handle GraphQL::Execution::Lazy instances (eg, GraphQL-Batch). Perhaps a similar approach can be made to work with Dataloader. But the challenge is cleaning up those placeholders. A Promise has its own catch block to do that, but Dataloader doesn't have that... (yet?)

Try setting placeholders and see what breaks

9d195d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return ordered result hashes #5060

Return ordered result hashes #5060

rmosolgo commented Aug 12, 2024 •

edited

Loading

	def gather_selections(owner_object, owner_type, selections, selections_to_run = nil, selections_by_name = {})
	selections.each do \|node\|
	# Skip gathering this if the directive says so
	if !directives_include?(node, owner_object, owner_type)
	next
	end

	if node.is_a?(GraphQL::Language::Nodes::Field)
	response_key = node.alias \|\| node.name
	selections = selections_by_name[response_key]
	# if there was already a selection of this field,
	# use an array to hold all selections,
	# otherwise, use the single node to represent the selection
	if selections
	# This field was already selected at least once,
	# add this node to the list of selections
	s = Array(selections)
	s << node
	selections_by_name[response_key] = s
	else
	# No selection was found for this field yet
	selections_by_name[response_key] = node
	end
	else
	# This is an InlineFragment or a FragmentSpread
	if @runtime_directive_names.any? && node.directives.any? { \|d\| @runtime_directive_names.include?(d.name) }
	next_selections = {}
	next_selections[:graphql_directives] = node.directives
	if selections_to_run
	selections_to_run << next_selections
	else
	selections_to_run = []
	selections_to_run << selections_by_name
	selections_to_run << next_selections
	end
	else
	next_selections = selections_by_name
	end

	case node
	when GraphQL::Language::Nodes::InlineFragment
	if node.type
	type_defn = query.types.type(node.type.name)

	if query.types.possible_types(type_defn).include?(owner_type)
	result = gather_selections(owner_object, owner_type, node.selections, selections_to_run, next_selections)
	if !result.equal?(next_selections)
	selections_to_run = result
	end
	end
	else
	# it's an untyped fragment, definitely continue
	result = gather_selections(owner_object, owner_type, node.selections, selections_to_run, next_selections)
	if !result.equal?(next_selections)
	selections_to_run = result
	end
	end
	when GraphQL::Language::Nodes::FragmentSpread
	fragment_def = query.fragments[node.name]
	type_defn = query.types.type(fragment_def.type.name)
	if query.types.possible_types(type_defn).include?(owner_type)
	result = gather_selections(owner_object, owner_type, fragment_def.selections, selections_to_run, next_selections)
	if !result.equal?(next_selections)
	selections_to_run = result
	end
	end
	else
	raise "Invariant: unexpected selection class: #{node.class}"
	end
	end
	end
	selections_to_run \|\| selections_by_name
	end

Return ordered result hashes #5060

Are you sure you want to change the base?

Return ordered result hashes #5060

Conversation

rmosolgo commented Aug 12, 2024 • edited Loading

rmosolgo commented Aug 12, 2024 •

edited

Loading