wundergraph / cosmo

The open-source solution to building, maintaining, and collaborating on GraphQL Federation at Scale. The alternative to Apollo Studio and GraphOS.
https://cosmo-docs.wundergraph.com/
Apache License 2.0
691 stars 100 forks source link

wgc CLI does not respect @inaccessible entities #901

Closed thekevinbrown closed 3 months ago

thekevinbrown commented 3 months ago

Component(s)

cli

Component version

0.94.2

wgc version

0.57.7

controlplane version

N/A

router version

0.94.2

What happened?

If possible, please create a PR with a failing test to illustrate the issue clearly. Otherwise, please attach a minimum reproduction through a GitHub repository that includes essential information such as the relevant subgraph SDLs. Please also make sure that the instructions for the reproduction are clear, tested, and fully accurate.

Description

One of my entities looks like this:

type AdminUiEntityAttributeMetadata @inaccessible {
  isReadOnly: Boolean
  exportPageSize: Float
}

When I run

wgc router compose -i compose.yaml -o config.json

I get this:

Failed to compose: Error: The object "AdminUiEntityAttributeMetadata" defines the same fields in multiple subgraphs without the "@shareable" directive:
 The field "isReadOnly" is defined in the following subgraphs: "music", "payments".
 However, it is not declared "@shareable" in any of them.

 The field "exportPageSize" is defined in the following subgraphs: "music", "payments".
 However, it is not declared "@shareable" in any of them.

As I understand it, this entity should not be processed by the supergraph at all, and should instead be completely ignored.

Steps to Reproduce

  1. Use @inaccessible on an entity in two subgraphs
  2. Run wgc router compose -i compose.yaml -o config.json

Expected Result

Expected successful composition omitting the entity.

Actual Result

Failed to compose: Error: The object "AdminUiEntityAttributeMetadata" defines the same fields in multiple subgraphs without the "@shareable" directive:
 The field "isReadOnly" is defined in the following subgraphs: "music", "payments".
 However, it is not declared "@shareable" in any of them.

 The field "exportPageSize" is defined in the following subgraphs: "music", "payments".
 However, it is not declared "@shareable" in any of them.

Environment information

Environment

OS: MacOS 14.5 (23F79) Package Manager: pnpm version 8.15.8

Router configuration

version: 1
subgraphs:
  - name: music
    routing_url: http://localhost:9001
    introspection:
      url: http://localhost:9001
  - name: payments
    routing_url: http://localhost:12346
    introspection:
      url: http://localhost:12346

Router execution config

No response

Log output

No response

Additional context

No response

github-actions[bot] commented 3 months ago

WunderGraph commits fully to Open Source and we want to make sure that we can help you as fast as possible. The roadmap is driven by our customers and we have to prioritize issues that are important to them. You can influence the priority by becoming a customer. Please contact us here.

thekevinbrown commented 3 months ago

Update:

I've also tried this:

type AdminUiEntityAttributeMetadata @inaccessible {
  isReadOnly: Boolean @inaccessible
  exportPageSize: Float @inaccessible
}

And I still get:

Failed to compose: Error: The object "AdminUiEntityAttributeMetadata" defines the same fields in multiple subgraphs without the "@shareable" directive:
 The field "isReadOnly" is defined in the following subgraphs: "music", "payments".
 However, it is not declared "@shareable" in any of them.

 The field "exportPageSize" is defined in the following subgraphs: "music", "payments".
 However, it is not declared "@shareable" in any of them.

To give you a bit more context as to why I'm using @inaccessible instead of @shareable:

This is part of Graphweaver. These entities are part of an API that is exposed on a query called _graphweaver that our Admin UI uses to introspect the entities and know what to show. If you have multiple Graphweaver instances behind Cosmo, they're each going to give completely different results, as each individual Graphweaver instance only knows about its own entities. For that reason I figured it wouldn't make sense to use @shareable. They're different data sets.

I have two options really:

  1. We could omit these entities entirely from the _service { sdl } response. This would allow composition to work, and really we don't need Federation knowing about these entities, so this could work.
  2. We can tell Cosmo about them and ask Cosmo to ignore them, which feels like the more honest way to go about things, so that's what I've opted for currently. Let me know if you agree or if I should pivot to approach 1.
Aenimus commented 3 months ago

Hi @thekevinbrown,

The behaviour shown is correct. What makes you think otherwise?

The @inaccessible directive applies to all instances of the definition to which it has been applied. If you declare Type.field @inaccessible in one graph, it is considered inaccessible in all graphs (consequently, Type.field will not be defined in the federated client schema). Pertinent to this issue, declaring a field as @inaccessible does not exempt the field from shareable constraints.

Is it possible that you are mistaken about the intended use of @inaccessible and @shareable? For instance, you said, and I quote:

If you have multiple Graphweaver instances behind Cosmo, they're each going to give completely different results, as each individual Graphweaver instance only knows about its own entities.

Fields of the same name on the same type are instances of the same field. An instance of a field must return the same data across all services, regardless of which service ultimately resolves that field. A router can resolve a shared field from two or more services, and which service it chooses depends on several factors, including efficiency. This means you can't really guarantee which service is going to resolve a particular instance of a field; it is all context-specific. Returning different data from different services should be considered a bug and is against the Federation protocol. The purpose of the @shareable directive is to enforce collaboration between teams for this exact reason (it solves an organizational problem with the intention to eradicate data disparity). You can read in more depth here (anchored to the most relevant section): https://cosmo-docs.wundergraph.com/federation/directives/shareable#overview

thekevinbrown commented 3 months ago

Yup entirely possible it’s my misunderstanding. Thanks for having a look and giving me more resources.