misleading composition hints when using @inaccessible

[dariuszkuc]

Given a simple schema definition

type ProductDimension @shareable {
  size: String
  weight: Float
  unit: String @inaccessible
}

We are getting misleading composition hints when other subgraphs do not include unit field:

rover HINT: [INCONSISTENT_OBJECT_VALUE_TYPE_FIELD]: Field "ProductDimension.unit" of non-entity object type "ProductDimension" is defined in some but not all subgraphs that define "ProductDimension": "ProductDimension.unit" is defined in subgraph "products" but not in subgraph "inventory".

Inaccessible fields may to be present in other subgraphs as they could be used for private keys, etc.

---

Repro available in apollographql/apollo-federation-subgraph-compatibility repo (apollo-server and federation-jvm implementations were updated to v2.3 tests).

To run tests, execute following make target from root dir:

make test subgraph=apollo-server

[pcmanus]

I don't care too much if we want to change this, but I also don't think the current behaviour is particularly wrong in the first place (so fwiw, I'd vote for keeping it, but mostly just on the argument of avoiding a change).

I suspect the underlying issue here, and what is truly misleading, is our UX around hints. And part of it is probably my fault for dropping the ball on apollographql/federation-rs#200, so I'll try to get that finished.

But it seems here that getting this is interpreted as misleading because nothing done is wrong, but this hint is always generated when nothing is wrong. Many do not highlight at problem, and that's the case of that one, they are just some information, that may or may not be useful to any particular user (we added levels to hints to try to convey this better, and the problem I linked to above is exactly the fact that those levels are not surfaced by rover at the moment). The hint here, INCONSISTENT_OBJECT_VALUE_TYPE_FIELD, only purpose is to highlight cases where federation 2 allows something that federation 1 wouldn't have (but so it only ever highlight cases that are valid).

To be clear, one could question how useful that hint is, and I would agree with that line of questions. This hint and a few others were added before fed 2 was released out of fear that some user would be surprised, and maybe dislike, some of the added flexibility of fed2, and it was an attempt at helping that. I have my own doubts on their particular usefulness, but that's probably a larger topic.

To the specific point here, @inaccessible was not really supported in fed1, so I think it ends up a bit arbitrary whether this hint is generated for them. More precisely, I think both of the following arguments are equally reasonable:

1. you could say that because inaccessible were not a thing in fed1, then we shouldn't have an hint;
2. or you could say that the point of the hint is to highlight fields for which fed1 would have required consistency, and if you had, say, manually declared @inaccessible, then fed1 would have rejected such field, and so showing the hint make sense.

Again, I don't personally care about which of those 2 arguments we want to use, I mostly think it's an unimportant choice and we should stick to the status quo. But I don't get the feeling this ticket was created to debate those 2 arguments, I suspect it was rather created on the misunderstanding that getting this hint is not in any way an indication that there is something wrong or invalid in the subgraphs.

[dariuszkuc]

I suspect it was rather created on the misunderstanding that getting this hint is not in any way an indication that there is something wrong or invalid in the subgraphs.

Ticket was created as those hints are just noise and could be misleading users that there is something wrong with the graph. Since @inaccessible wasn't available in fed 1, we should be looking at it from fed 2 perspective.

AFAIK there are two major use cases for usage of @inaccessible:

1. private keys

In this case, my intent is to never have those fields exposed in the supergraph and the "hint" is just a noise.

2. schema evolution

If I'm slowly evolving my schema by adding @inaccessible fields to each individual subgraphs, it may make sense to keep the hint as giving me heads up that there are still some other subgraphs out there that don't have this field yet. Its debatable how useful this hint is but it "may" be useful in some cases.