Requisite Fields and Query Subtraction

[skevy]

I noticed an odd bug when my Relay application was trying to fetch the following two fragments in a parent and child component, respectively:

fragment on Viewer {
  id,
  activeTicket {
    id
  }
}

fragment on Viewer {
  id,
  paymentMethods {
    id
  },
  locations(first: 20) {
    edges {
      node {
        id
        name
        address
        city
        state
        zipCode
        imageUrl
      }
    }
  },
  activeTicket {
    id
    authed
    paid
  }
}

The queries that actually get issued to my GraphQL backend are as follows:

query ViewerQuery{
  viewer{
    id,
    ...__RelayQueryFragment01ktc8t
  }
} 

fragment __RelayQueryFragment01ktc8t on Viewer{
  id,
  activeTicket{
    id
  }
}

and

query ViewerQuery{
  viewer{
    id,
    ...__RelayQueryFragment16p69yi
  }
} 

fragment __RelayQueryFragment16p69yi on Viewer{
  id,
  _locationsik5bcm:locations(first:20){
    edges{
      node{
        id,
        name,
        address,
        city,
        state,
        zipCode,
        imageUrl
      },
      cursor
    },
    pageInfo{
      hasNextPage,
      hasPreviousPage
    }
  },
  activeTicket{
    id,
    authed,
    paid
  }
}

A keen eye may notice that the paymentMethods field is missing from the second query that's sent to GraphQL.

This seemed extremely odd to me, so I dove in a bit (and by dove in, I mean stepped through the whole RelayQuerySubtractor). It turns out that what was happening is that the id field of the paymentMethod field on Viewer was being marked as a requisite field by the Babel plugin, so when it checks if paymentMethods is empty, as it does the query subtraction, it thinks that paymentMethod is empty (the code I'm referencing is here:

relay/src/traversal/subtractRelayQuery.js

Lines 184 to 198 in 4e671a3

	function isEmptyField(
	node: RelayQuery.Node
	): boolean {
	if (node instanceof RelayQuery.Field && node.isScalar()) {
	// Note: product-specific hacks use aliased cursors/ids to poll for data.
	// Without the alias check these queries would be considered empty.
	return (
	node.isRequisite() &&
	!node.isRefQueryDependency() &&
	node.getApplicationName() === node.getSchemaName()
	);
	} else {
	return node.getChildren().every(isEmptyField);
	}
	}

).

Interestingly, there's that sneaky comment in there:

"Note: product-specific hacks use aliased cursors/ids to poll for data. Without the alias check these queries would be considered empty."

Sure enough, when I either do:

paymentMethods {
  paymentMethodId: id
}

or

paymentMethods {
  id,
  last4
}

it requests the paymentMethods list with the id correctly.

Note, that in my schema, paymentMethods is just a normal GraphQLList type, not a connection.

So, TLDR; is this desired and correct behavior? If so, what is the reasoning behind it? What problem is this solving? We should somehow figure out how to document this behavior...because I feel like in general, without a super deep knowledge of Relay and how the Babel plugin works, it would be very difficult to understand why this was happening.

If it ISN'T desired and correct behavior, it seems that it needs to be fixed in here -

relay/scripts/babel-relay-plugin/src/RelayQLPrinter.js

Lines 152 to 154 in 4e671a3

	if (fragmentType.hasField('id')) {
	requisiteFields.id = true;
	}

. I noticed that, in other places in the printer, it's much more careful about how it sets a field to be requisite:

relay/scripts/babel-relay-plugin/src/RelayQLPrinter.js

Lines 264 to 268 in 1e2067b

	if (parentType.isConnection() &&
	parentType.hasField('pageInfo') &&
	fields.some(field => field.getName() === 'edges')) {
	requisiteFields.pageInfo = true;
	}

Thanks, and sorry for the long issue!

[wincent]

Thanks for bringing this up, and in such a well-detailed way!

It's possible that this issue will simply go away in the future because we as almost certainly going to remove query subtraction as a concept from Relay.

So, TLDR; is this desired and correct behavior? If so, what is the reasoning behind it? What problem is this solving?

In short, yes, and I'll try to explain why.

Why we subtract queries now

The idea of query subtraction is that if you fire off multiple queries in a batch, later queries in the batch may redundantly request fields that prior in-flight queries from the batch are already in the process of retrieving. So, we subtract any in-flight queries from newly-added queries before sending them (the newly-added queries). Sometimes, these queries can get subtracted away to nothing, in the sense of them being "empty", which means there's no point in sending them. Here's an example of an obviously "empty" query:

query MyQuery {
   node(id: "foo") {
     id
  }
}

In this query, we already know the id, so there is no point in requesting it.

An aside on requisite fields

Note that there are a few different field types at play here:

• requisite: Without these fields, Relay won't work. They should not be stripped.
• generated: These are generated by Relay whenever we notice a place where we'll need them (ie. they are also requisite) but the query author hasn't explicitly provided them.
• normal: Fields supplied by the query author (which means non-generated, although they may be requisite).

So, a field can be:

• requisite and generated
• requisite and non-generated
• non-requisite and non-generated

But not non-requisite and generated (we wouldn't have generated it if it weren't requisite).

Back to what I was saying about subtraction...

As you saw, we are going to consider a field to be "empty" if, after subtraction, it is a scalar (no children), it's requisite, it's not a "ref query dependency" (this is related to our support for deferring part of a query, and these fields can never be stripped), and it's not aliased. That aliasing check is the escape hatch, and the nasty hack that you discovered to hint to the subtractor that it shouldn't consider your field to be content-less and unworthy of sending.

We do all of this so that a tree that consists of only requisite fields and their parents may be considered empty, because there is no demonstrated reason why you would want to fulfill such a query in practice. If we were to let these through, we'd end up letting a lot of basically empty/pointless queries slip through to the server for no good reason.

Why we think it may not be worth subtracting queries in the future

In practice, we suspect that the actual amount of data-over-the-wire that we save by doing this subtraction is minimal. We could probably just delete the whole thing, get a nice reduction in complexity, with little or no impact on performance (any over-the-wire costs could be offset by cheaper processing costs, an easier-to-iterate code base, and scope for doing other clever things to reduce query upload size).

The only reason we need this still, for now, is to support the "preload" mode that @voideanvalue describes here. In this mode, we run queries on the server, the client starts downloading static resources, we tell the client about the queries and it registers them as pending. The query results come in and we stream them down. In the meantime, the client can use query subtraction to ensure that it doesn't redundantly request data which is already being fetched by "in-flight" queries (even if they are in-flight on another machine, in a data center).

There are other ways to solve this problem without relying on query subtraction, but that is the (temporary) solution that we have for now.

Recommendation

You should probably run with your alias hack for now, and know that this problem will eventually go away. I don't know if we actually want to document this right now (beyond what I've written here), because I am hoping that this is all just temporary.

As this is not a bug but is working as intended, I'm going to mark this as closed, but feel free to add comments with any more questions or thoughts that you might have.

[taion]

@wincent Why is the correct behavior in that check not to only strip off the generated fields rather than all the requisite ones? It seems like in practice you'd get all the relevant benefits if you considered only the generated ones, while offering a nicer and less unintuitive escape hatch in the case that someone actually wants to query for e.g. widgets { id } per the above example (where we have widgets: [Widget], so the query is not actually completely pointless - gives a generic, if potentially expensive, way to get the length of the list).

[taion]

Ah... I guess it handles the case where one query wants e.g. widgets { name } and another wants widgets { id, name }... oh well.