Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Data masking with fragments #11666

Open
jerelmiller opened this issue Mar 13, 2024 · 19 comments
Open

RFC: Data masking with fragments #11666

jerelmiller opened this issue Mar 13, 2024 · 19 comments
Assignees
Labels
Milestone

Comments

@jerelmiller
Copy link
Member

jerelmiller commented Mar 13, 2024

This work is part of the Data masking milestone. Follow the milestone for progress on this feature.

Background

When building complex applications today, Apollo Client users reach for useQuery to get data into their components. This is no surprise as this is considered a best practice by the Apollo Client documentation.

We are taking an initiative this year to change the recommendation to instead prefer fragment composition and fragment colocation as the standard pattern for building apps with Apollo Client. While the out-of-the-box experience with this pattern works, as recommended by our documentation, there are a few shortcomings of the existing solution:

  • It’s easy to introduce implicit coupling on fragment data from query components1. This makes the app more prone to breakage as child components are refactored and data requirements change.
  • Cache writes rerender at the query component level, making fine-grained updates for fragment components difficult to do2

To alleviate these shortcomings, we are are introducing data masking into Apollo Client. Data masking is popularized by Relay and is useful to avoid implicit dependencies between components.

What is data masking?

Data masking is the functionality that provides access only to the fields that were requested in the component. This prevents implicit coupling between components by allowing components to access only the fields requested by that component. For query components, this includes all fields in a query not included in a GraphQL fragment. For fragment components, this includes the fields defined in the fragment definition.

Take the following as an example:

const query = gql`
  query {
    user {
      id
      name
      ...UserFields
    }
  }
`;

function App() {
  const { data } = useQuery(query)
  
  // loading state omitted for brevity
  
  return (
    <div>
      {data.user.name}
      <User user={data.user} />
    </div>
  )
}

function User({ user }) {
  // ...
}

User.fragment = {
  user: gql`
    fragment UserFields on User {
      age
      birthdate
    }
  `
}

In Apollo Client today, all user data is available to <App />. This means that <App /> can access fields such as age and birthdate, even though these fields were asked for by the fragment in Child. This creates an implicit coupling between <App /> and <User />. For example, if <App /> consumed user.age, and <User /> was refactored to remove age from the fragment, this would break the <App /> since age will no longer be loaded by the query.

Data masking solves this by only allowing the fields declared in that component to be accessible in the component that asked for that data. In this example, <App /> would only have access to user.id and user.name, but not user.age and user.birthdate since these were part of the fragment. <User /> would have access to user.age and user.birthdate, but not user.id or user.name since these fields are not part of the fragment.

The same applies to fragments that include nested fragments. One fragment cannot access data defined in a nested fragment. Take the following example:

const UserFragment = gql`
  fragment UserFragment on User {
    id
    name
    ...UserProfileFragment
  }
`

const UserProfileFragment = gql`
  fragment UserProfileFragment on User {
    age
    birthdate
  }
`

Here UserFragment can access id and name, but not age and birthdate. UserProfileFragment can access age and birthdate, but not id and name since these fields are not declared in the fragment.

Usage

With data masking enabled, you will issue queries the same as you do today. This is typically done with the useQuery or the useSuspenseQuery hook if you've adopted Suspense. The difference is that you will not be able to access fields defined in fragments from these components.

Instead, fields declared in fragments will be accessed through the useFragment hook*. To provide a nice developer experience, we plan update the from option in useFragment to support passing the entire parent object that contains the fragment as the value to this option. See the "Example" section below for a full code sample.

*Depending on technical feasibility and backwards compatibility, we may need to introduce a separate hook. This will be part of the exploratory work of this feature.

@defer

As part of this work, we will integrate useFragment with @defer and detect when the fragment is in-flight. This will integrate with React Suspense and cause the component to suspend until the fragment data has loaded.

Usage with non-suspenseful hooks

useFragment will not be limited to usage with suspenseful hooks however (such as useSuspenseQuery). Users may not yet be compatible with Suspense or may primarily be using useQuery to power their apps. Enabling suspense in these situations would be unwise and induce frustration.

To avoid this, we plan to make useFragment aware of the hook that produced the query to conditionally suspend the component. This means that Suspense will only be available when the query is produced by a suspenseful hook such as useSuspenseQuery.

Fetch policies with cached data

Users have the ability to leverage fetch policies to determine how to use cached data when consuming a query. For example, you can bypass the cache and force a network request with network-only, or read from the cache while fetching from the network with cache-and-network.

useFragment should be aware of this to mimic the query hook behavior when determining whether to return a cached result or suspend. For example, when a deferred fragment is used within a network-only query, the hook should suspend until the fragment is fulfilled, regardless of whether there is data in the cache. When used with a cache-and-network query, useFragment should provide the cached data and rerender when the network request finishes.

Non-React frameworks/libraries

The Apollo Client ecosystem is not limited to just React users. Libraries such as Apollo Angular and Vue Apollo provide view bindings for their respective view libraries. We plan to provide the foundation for these libraries to adopt this feature as well. This extends to users that use Apollo Client's core APIs.

We will be layering much of this work into Apollo's core query APIs, such as watchQuery and v3.10's watchFragment.

Render performance

Adopting data masking will include more benefits than just programming best practices. It will also provide performance benefits.

Today, cache writes to fields in a fragment definition cause the query component to re-render, regardless of whether that component consumes the data from the fragment or not. Depending on the depth of the component tree mounted beneath the query component, this may have significant performance implications. Many users avoid this by introducing additional query hooks in components further down the component tree. This however comes at the cost of additional network requests.

You can avoid the render performance implications today with the use of the @nonreactive directive combined with useFragment3. While this works well, it requires manual intervention.

With data masking enabled, this performance benefit will be an out-of-the-box feature. Because useFragment will be required to read data out of a fragment, we will target re-renders on the fragment components directly when there are cache writes to fields in the fragment.

Enabling this feature

Once this feature is introduced, it cannot be enabled automatically since this would constitute a breaking change. Instead, we will need to allow an opt-in to this feature. We plan to allow this in 2 ways:

Globally

We will allow data masking to be opted into globally by introducing a new option to ApolloClient. Enabling this option would automatically turn on data masking for every query that uses the client instance.

new ApolloClient({ 
  // This name is subject to change
  dataMasking: true
})

We will recommend that new users and applications opt into this feature immediately as the default. Smaller apps that have the capacity to migrate in an afternoon should also consider enabling this feature. We plan to make this the default in future major versions of Apollo Client with the eventual goal to deprecate this option and make this standard behavior.

Incrementally

We understand that large apps cannot stop everything and adopt this feature in its entirely. To allow large apps to migrate over time, we will allow this feature to be opted into incrementally.

We will do so by first requiring that users enable data masking globally, then allowing users to opt-out of data masking per named fragment. While this approach seems counterintuitive to the goal of an incremental migration, this approach has some advantages:

  • New queries are automatically masked. This takes out possible human error forgetting to enable data masking on any newly introduced query to the application.
  • Over time, you are removing code to adopt data masking rather than adding code. This makes it easy to spot which queries in your application have not yet adopted the feature.

To make this approach feasible at scale, we will provide some out-of-the-box tools to handle the up-front work for you. For more information, see the “Migration tools” section below.

@unmask

We plan to add support for a new client-only directive @unmask that marks a named fragment as unmasked.

query MyQuery {
  user {
    id
    ...UserFields @unmask
  }
}

Named fragments marked with @unmask will behave as it does today, allowing access to all fields, including those defined in fragments. We are making this a directive used on named fragments to make it easier for you to migrate specific subtrees that consume data from named fragments.

@unmask migration mode

While @unmask works suitably on its own, it can be difficult to determine at any given time how many of your query components consume fields from named fragments that would normally be masked. We want to provide an obvious way to identify areas of your code where you've accessed a field that would normally be masked. We're introducing a migration mode for @unmask that will warn in development when a would-be masked field is accessed in your components. To enable migration mode, you'll set the mode argument:

query MyQuery {
  user {
    id
    ...UserFields @unmask(mode: "migrate")
  }
}

Once you no longer see warnings in your code, it should be safe to remove the @unmask directive to start masking data for fields defined in the named fragment.

Migration tools

We will provide utilities that will ease migration in large apps to make it feasible to adopt this feature.

  1. Codemod

We will provide a codemod that will crawl through the application and apply @unmask fields to every named fragment. This will handle queries used in gql tags and .graphql/.gql files. By default this will apply the @unmask directive in migrate mode, but there will be an option not to apply migrate mode in case you want to avoid the warnings.

  1. ESLint plugin

We will provide an ESLint plugin with a rule that will warn for queries that contain @unmask directives used in migrate mode. This provides a more automated way to see areas of the codebase that have not yet adopted data masking. This also makes it possible to ban usage of the directive once data masking is fully adopted.

Example

const USER_QUERY = gql`
  query UserQuery {
    user {
      id
      name
      ...UserInfoFields
      ...UserAvatarFields
    }
  }
`

function App() {
  const { data } = useQuery(USER_QUERY);
  
  return (
    <div>
      {data.user.name}
      <UserInfo user={data.user} />
      <UserAvatar user={data.user} />
    </div>
  );
}
 

const USER_INFO_FRAGMENT = gql`
  fragment UserInfoFields on User {
     age
     birthdate
  }
`;

function UserInfo({ user }) {
  const { data } = useFragment({ 
    fragment: USER_INFO_FRAGMENT,
    from: user
  })

  return (
    <div>
      {data.age} - {data.birthdate}
    </div>
  ) 
}

const USER_AVATAR_FRAGMENT = gql`
  fragment UserAvatarFields on User {
    avatar {
      url
    }
  }
`;

function UserAvatar({ user }) {
  const { data } = useFragment({ 
    fragment: USER_AVATAR_FRAGMENT,
    from: user
  });

  return <img src={data.avatar.url} />
}

Open questions

Should we allow this for queries that use no-cache fetch policies?

useFragment is a cache API. It allows you to selectively read data out of the cache and re-render when that data changes. This gets tricky when used with no-cache queries. We'll either need add support to useFragment to allow it to be used without the cache, or we'll need to prevent usage with no-cache queries.

Depending on our final decision, this may warrant the introduction of a new hook to distinguish useFragment as a cache-only API.

Should this feature apply to cache.readQuery and cache.readFragment?

Cache APIs, such as cache.readQuery and cache.readFragment can be thought of as selectors for data in the cache. Data masking makes less sense here if you just need to read some arbitrary data out of the cache, especially since the queries/fragments you provide to these APIs do not actually have to be a query that was previously executed on the network. These APIs do not cause network requests and do not play a role in re-rendering your components.

Instead, we may prefer to build this into the client layer between the cache and the end usage. For example, using client.watchQuery and client.readQuery would be data-masked. You'd pair this with client.watchFragment and/or client.readFragment to masked fragment data from these APIs. The inherent risk with this approach is that it may cause confusion since the distinction between client.readQuery and client.cache.readQuery may not be apparent.

Can we allow fragment selections on non-normalized data?

Due to the way fragments work with the cache, useFragment is only able to read normalized data out of the cache. The from option creates the cache key used to look up the entity in the cache. With the planned update to the from option, should it be possible to read non-normalized data via useFragment?

The downside to allowing this is potential confusion on when this is allowed. Allowing non-normalized data to be selected when from originates from a query may make it feel like you can do this with any random fragment.

Will it be possible to pass the parent objects provided to from in React context, reactive vars, etc.?

It should theoretically be possible to pass around the parent objects that would normally be passed to child props in React context or other means of transporting values. I'm capturing this as an open question to make sure we are thinking about this while developing the feature.

How do cache writes work with data masking?

With data masking enabled, cache reads and writes are no longer symmetrical. We will need to explore ways to make cache.writeQuery make sense with this new paradigm.

Footnotes

  1. Query components meaning components that initiate a GraphQL network request via a query hook, such as useQuery.

  2. This can be avoided with the combination of @nonreactive and useFragment today, but is more subject to error as it requires you to manually add @nonreactive in the appropriate places.

  3. For a more in-depth look at this feature, see @alessbell's blog post titled "Don’t Overreact! Introducing Apollo Client’s new @nonreactive directive and useFragment hook".

@jerelmiller jerelmiller self-assigned this Mar 13, 2024
@jerelmiller jerelmiller pinned this issue Mar 13, 2024
@jerelmiller jerelmiller added this to the Data masking milestone Mar 13, 2024
@adamesque
Copy link

A few thoughts:

  • Love the proposed change to from. If this is intended to allow fragment-reading functions to work with fragments defined on non-normalized types, that would be great to capture in the RFC (and I would love it)!!
  • It isn't explicitly stated, but I presume that data masking would also apply to data returned from useFragment et. al when the passed fragment contains nested fragments. If that's the case, might also be good to call out, especially if that means fragments would also need to be tagged with @unmasked during the transition
  • In our environment, graphql-codegen is widely used and has its own fragment masking features as part of its client-preset. It might be helpful to have a migration guide available.

@yaboi
Copy link

yaboi commented Mar 13, 2024

Are there any other ideas or suggestions on how to pass the parent object around beside React props? For example, if the component tree has a query at a top level but the colocated fragment isn't used for a series of nested children components? An example of this might be outside of the RFC here, but one of the things I like so much about useFragment right now is how it has removed the need for us to pass data around via React props.

@adamesque
Copy link

We'll either need add support to useFragment to allow it to be used without the cache, or we'll need to prevent usage with no-cache queries.

Would the proposed enhancement to from potentially help solve for this case?

Something we've wanted for a while is automatic query registration for fragments so that a loading flag (or suspense state) could be surfaced via useFragment when any query containing that fragment is in flight. If you were doing that kind of registration, you could potentially warn if an attempt is made to use useFragment with a no-cache query without passing the fragment data directly via the enhanced from arg.

@jerelmiller
Copy link
Member Author

@adamesque thanks so much for the feedback!

If this is intended to allow fragment-reading functions to work with fragments defined on non-normalized types, that would be great to capture in the RFC

I would LOVE for this to be the case, though I'm still a bit technically uncertain if this is going to be possible.

I've added this as an open question to make sure this is captured. This is definitely going to be explored as a possibility.

I presume that data masking would also apply to data returned from useFragment et. al when the passed fragment contains nested fragments

YES! Thanks for calling this out! I've added this to the section on "What is data masking?" to make sure this is captured.

In our environment, graphql-codegen is widely used and has its own fragment masking features as part of its client-preset. It might be helpful to have a migration guide available.

Yes good call! Codegen compatibility is important, but capturing the differences between codegen's useFragment helper and Apollo's built-in data masking will be important as well. Thanks for calling this out to make sure we are thinking through this as we develop the feature.

Would the proposed enhancement to from potentially help solve for this case?

Thats the goal, though really what I get stuck on is the "confusion" aspect of this potentially. Our docs start with marketing useFragment as this:

The useFragment hook represents a lightweight live binding into the Apollo Client Cache

While its certainly possible that we change the "marketing strategy" on this hook, I just want to make sure that everything feels intuitive, otherwise we might end up with more frustration than solved problems.

I think this is just a matter of building it to work this way, then trying it out to see where the papercuts are. I plan to fully try out the new capabilities on our spotify showcase to make sure I can see it through the lens of the consumer. If all else fails, then we'll add a new hook to capture all of this functionality (though I'd prefer not to... naming is hard 🤣).

so that a loading flag (or suspense state) could be surfaced via useFragment when any query containing that fragment is in flight

This is planned as a followup to the data masking work 🙂. The data masking work should lay the ground work for us to determine when fragments are in flight so that we can layer in suspense functionality.

@jerelmiller
Copy link
Member Author

@yaboi thats a great question! Any updates we do to useFragment will HAVE to be backwards compatible, otherwise its a breaking change. Everything you do today in terms of passing this around via context, reactive vars, etc. will continue to function as it does.

I really see the updates to from as more of a "when you use it this additional way, it adds these behaviors". BUT this is a good callout because it should theoretically be possible to put that parent object that you'd pass via props into React context or a reactive var and still get the same behavior. I'll add this as an open question though to make sure its captured in the RFC.

@ghost
Copy link

ghost commented Mar 23, 2024

Heyo, would love to see a small example of a unit test using the new from: option

Right now we're using cache.writeFragment + useFragment that just use an ID to introspect the cache.

In order to keep our tests atomic, its important we be able to mock the kind of object reference that goes into "from" easily

@jerelmiller
Copy link
Member Author

@blasterfiles thats a great callout! I've added #11735 to track this. Thank you!

@stubailo
Copy link
Contributor

Hi! This looks really cool. One question I had is, what will the TypeScript types look for user in this instance? How would one type "this prop is a thing I need to pass into useFragment", in a way that also ensures that the parent component passes in the right object?

@vladar
Copy link
Contributor

vladar commented Apr 11, 2024

We experimented a little bit with a custom @mask directive, which works similar to @skip at the cache reading level. This proved to be a pretty simple implementation (just a few line change in the readFromStore) and it showed a nice performance boost - both for the initial useQuery and subsequent updates (because unlike @nonreactive the result of useQuery isn't changing in optimism on fragment fields changes, so the cache doesn't even invoke watcher callback for parent useQuery).

With this directive it was also trivial to adjust GraphQL codegen to exclude fragment fields from generated types. So, it could be easier to migrate exiting apps with such "opt-in" approach rather than "opt-out" with @unmasked?

Funny enough, we still found @nonreactive valuable for some of our scenarios where we wanted to keep some fields in the result of the parent useQuery, while preventing them from triggering re-renders on data change.

@jerelmiller
Copy link
Member Author

@stubailo to be honest this is still a bit of an unknown right now. We are absolutely aware that we need TS types to align nicely with the updates to useFragment here, but I cant't say we know exactly what that will look like quite yet. Stay tuned!


@vladar thats interesting to hear! Glad to hear you've had a good experience a masking-type solution! I'll be happy to have something first-class in the client as well here so that everyone can benefit 🙂.

So, it could be easier to migrate exiting apps with such "opt-in" approach rather than "opt-out" with @unmasked?

We've had discussions about the approach here and realize that an opt-out solution adds a bit of investment up-front, especially for large apps. I'll restate what I put in the RFC though on why we believe this is the right direction:

this approach has some advantages:

  • New queries are automatically masked. This takes out possible human error forgetting to enable data masking on any newly introduced query to the application.
  • Over time, you are removing code to adopt data masking rather than adding code. This makes it easy to spot which queries in your application have not yet adopted the feature.

We plan to make this process easier with migration tools so that its a more automated process. I strongly believe though the long-term benefits will outweight the up-front cost to enable it, even if your app remains in a state of "migration" for a long-period of time. This approach also prepares for a future where data-masking will become the default. This likely won't happen for another couple majors, but this is the long-term goal.

Funny enough, we still found @nonreactive valuable for some of our scenarios where we wanted to keep some fields in the result of the parent useQuery, while preventing them from triggering re-renders on data change.

That's very interesting! In my mind, data-masking will remove the need for @nonreactive in most cases, so its cool to hear you've still found uses for it!

@rbalicki2
Copy link

Absolutely love it!! Especially the thought that went into incremental adoption. I have a bunch of thoughts, mostly related to wording in this RFC, but some related to avoiding what I feel are mistakes that Relay made.

Overall, data masking is great and I think this is awesome, and I can't wait to see what y'all ship.

Clarity about when fragments suspend

  • "detect when the fragment is in-flight". Fragments are not in flight. Maybe it would be more accurate to say e.g. "if the query from which the fragment was sourced is in flight while the fragment reads missing server data." And this makes clear that it's not that simple. What about:
    • queries that are re-executed?
    • queries that refetch part of the same tree?
    • What about mutations that have overlapping fields? etc.
    • fragments whose fields are not missing (i.e. they were previously cached), but whose fragment is deferred?
  • The reason I bring this up is that you may encounter relationship changes leading to missing data. For example, a fragment selects best_friend { name }, and a follow up query selects best_friend { age } and incidentally discovers a new best friend. Now, you have a fragment with missing data.
    • In this situation, will random unrelated queries cause this fragment to suspend? This is problem that occurs in Relay.
  • I think the connection to deferred fragments is incidental. A deferred fragment is a fragment for which there is an outstanding query and missing data. But that's not the only situation that that arises! Consider a query containing a fragment Foo on Query { node(id: $id) { __typename }}. The parent query is refetched with a new $id. The Foo fragment now has missing data and an outstanding query.

Whether a fragment suspends should be a fragment-level choice

  • In relay, the fetch policy also controls whether fragments suspend. Or at least, that's true for the root query.
  • Fetch policies conflate two things: whether to fetch and whether to suspend. e.g. network-only is fetch yes and suspend yes, while store-and-network is fetch yes, suspend no.
  • It would be conceptually cleaner to separate these two.
  • Anyway, suspense occurs at the fragment level (i.e. based on whether a fragment is reading missing data), so you should be able to control whether you want to suspend on a fragment-by-fragment basis.
    • A good default would be to inherit the suspense policy from the parent, but I haven't thought it through.
  • In my case, being able to opt out of fragment-level suspense would make adoption of Relay 100x easier, as we are migrating from Redux to Relay, and our Redux setup doesn't support suspense. Adding suspense causes us to show fallbacks instead of "optimistically showing what data we can", and this causes regressions in our metrics.
  • An added benefit of this might be in terms of type generation. If a fragment has @willAlwaysSuspendIfAnyDataIsMissing, you can generate types that are non-optional. But if a fragment has @willNeverSuspend, you can generate types that are optional. And then, enforce with the type system that you pass the appropriate suspensePolicy to useFragment.
  • This is conceivably better for incremental adoption, as well.

Data masking and denormalized data

  • At Pinterest, we are migrating from a Redux store (w/REST) to Relay/GraphQL. Our general plan is to implement Relay-compatible APIs that provide fragment data masking and can read from either the Relay store (i.e. they call the Relay APIs) or from denormalized data. This will allow us to incrementally adopt Relay APIs, and once a tree is fully migrated, turn on GraphQL and Relay.
  • When reading from denormalized data, we "stash" the original data in a special symbol, i.e. if you read out { name, ...ChildFragment } you might receive { name: 'John', [secretSymbol]: DenormalizedData }
  • So, data masking and denormalized data is possible. Happy to dive into more detail (e.g. about pagination, etc.) if you're interested

Prop drilling of fragment references

  • Everyone also has a negative first impression if they see fragments with only fragment spreads. But this is fine! It's cheap, and preserves future flexibility.
  • IMO your docs should heavily emphasize that this is a best practice, or people will fall into a pit of a failure where they pass props down multiple levels or store fragment references in context.

@jerelmiller
Copy link
Member Author

@rbalicki2 first off, thank you so so much for this incredible feedback! There are definitely use cases that I hadn't quite thought of that will make excellent test cases. I'm going to take a bit of time and digest some of this and respond back with some of my thoughts when I've thought this through a bit more. I sincerely appreciate this!

@vladar
Copy link
Contributor

vladar commented Apr 30, 2024

@jerelmiller Thanks for your response, let me reply inline

We've had discussions about the approach here and realize that an opt-out solution adds a bit of investment up-front, especially for large apps.

For large apps it is indeed important to be able to experiment with the feature on a small subset of the codebase and once proven useful - gradually introduce it elsewhere. But I don't think opt-in vs. opt-out are mutually exclusive. In my mind the transition could be done in multiple stages:

  1. Introducing an opt-in way in a minor version (e.g. @mask directive). This will significantly simplify adoption for big apps, as it is a non-breaking change, doesn't require major upgrade, and allows people to experiment with it and "feel" the benefits.
  2. For the next major - switch to opt-out approach (but maybe still let people who decided to opt-out to have @mask supported for selected parts of the app)
  3. For the subsequent major - make it the default with @unmask as the only way to opt-out

Such a gradual approach will make adoption much smoother and will also let you receive feedback earlier. Also, it doesn't conflict with the other stated benefits after major upgrade:

  • New queries are automatically masked. This takes out possible human error forgetting to enable data masking on any newly introduced query to the application.
  • Over time, you are removing code to adopt data masking rather than adding code. This makes it easy to spot which queries in your application have not yet adopted the feature.

Challenge with manual writes

Another challenge that would be great to cover in this RFC is manual writes to cache of operations containing masked fragments. This is a somewhat interesting topic, especially for strictly typed environments.

With data-masking reads and writes become non-symmetrical. Writes expect full data as input, whereas reads return partial data by default. This is a sort of ambiguity that could be resolved with different trade-offs.

(it is somewhat related to the section on manual cache.readQuery and cache.readFragment, but writes are even more tricky)

@jerelmiller
Copy link
Member Author

@vladar appreciate the observation on cache writes! I've added this as an open question for now to ensure this is captured. I think this will take a bit of exploration and experimentation to get right.

In my mind the transition could be done in multiple stages

I think you and I are aligned in the goals here which is ultimately to make data masking the default behavior. Where your thinking differs from mine is the approach to get there. My thinking was that changing the default essentially means flipping the dataMasking flag from false -> true in a major version, then removing the flag altogether a major version after that. I worry that with your approach, its a bit of whiplash (i.e. first add @mask, then in the next major, swap all those for @unmask, though perhaps I'm misunderstanding your thinking here). The more friction we can avoid, the better. Doing the work up-front to globally enable and add @unmask everywhere is a one-time up-front cost, then from there its deleting code to adopt the feature (more on that below).

That being said, you make a good point here:

it is indeed important to be able to experiment with the feature on a small subset of the codebase and once proven useful - gradually introduce it elsewhere

Experimentation can be important, especially if the codebase spans multiple teams (common in large enterprise organizations). Many of these teams likely have processes in place where major changes like this need a vetting period where perhaps a team lead can create a proof-of-concept and pitch it to the rest of the org. I definitely don't want to ignore this.

I've entertained the idea of allowing a @mask directive as you mentioned, but I still have some reservations about this approach. Let me break down my points about this a bit further:

New queries are automatically masked.

This to me is a big one and where my goal is to try and ensure that once you've decided to adopt it, you're removing as much human error as possible. The @mask approach only works if you remember to add that directive for any new query added to your system. If you or someone on your team forgets this directive, this can add more work after-the-fact as it means having to revisit this query, add the directive, and test to ensure that unmasked fields aren't used in the wrong places. Hopefully this could be caught in a code review, but again, we're all humans and prone to make mistakes (especially if you are under a time crunch trying to deliver on a feature).

It can be incredibly helpful if Apollo Client can help drive adoption naturally without much thought or process on your end.

Over time, you are removing code to adopt data masking rather than adding code

Again to reiterate, if your team adopts the feature by using @mask rather than dataMasking: true + @unmask, you're 1) adding code to incrementally adopt this feature and 2) once you've fully adopted data masking across all your queries, you're having to go back through and remove the @mask directives in your existing queries. Furthermore, its difficult to tell at any given time whether or not you've actually fully adopted masking since it would require you to audit all your queries for a @mask directive. Any query without a @mask has the potential to break once you flip dataMasking to true.

This is what I mean by "removing code to adopt data masking". By default you're starting with @unmask, so to mask a query, you're removing the directive. Once you're removed all @unmask directives, there is no more work for you to do and you've fully adopted it across your codebase.

I find this approach easier to add linting rules to as well. During the transition period, you could set the linting rule to warn on @unmask usage. Once you've fully adopted masking (i.e. you've removed all @unmask directives), you switch that lint rule to error which helps prevent new queries from sneaking in @unmask.


At the end of the day, my goal here with this approach is to try and help teams avoid mistakes when adopting this feature. I'm willing to admit that I might be trying to push too hard toward the "pit of success" though.

I'll entertain the idea of adding a @mask directive or some other way to incrementally adopt in this manner. If we go that route, we'll likely recommend that you only use this for experimentation and prefer the @unmask approach with all the reasons I listed above. Again, I do think your point about experimenting on a subset of queries to get a feel for it is important and can be useful to prove out adoption. I don't want to dismiss this idea.

Appreciate the insight and conversation here!

@vladar
Copy link
Contributor

vladar commented Jun 13, 2024

@jerelmiller I've opened a draft PR with reference implementation for @mask in case if you want to play with it: #11896

It could be used as a direct replacement for @nonreactive directive.

@unbelivr
Copy link

How this is supposed to work with React Server Components? Adding 'use client' directive to children components just to unmask with useFragment seems like overkill. It would be useful to have similar non-hook API.

async function App() {
  const { data } = await apolloClient.query({ query: USER_QUERY });

  return (
    <div>
      {data.user.name}
      <UserInfo user={data.user} />
    </div>
  );
}

function UserInfo({ user }) {
  const { data } = getFragment({ 
    fragment: USER_INFO_FRAGMENT,
    from: user
  })

  return (
    <div>
      {data.age} - {data.birthdate}
    </div>
  ) 
}

@jerelmiller
Copy link
Member Author

@unbelivr we are still working on a story for RSC in this regard, especially because it has an effect on Client Components. Our initial release of data masking will be on the core @apollo/client package with support for the streaming packages to come after that.

One of the prerequisites to allowing this in Server Components is to figure out if/how we want this feature to work with no-cache queries. useFragment as it stands in our feature branch is still dependent on the cache, which means we need to create another mechanism to read fragment data from no-cache queries. This mechanism will likely be the same one we'd use to provide a helper method for Server Components to read that fragment data.

For the initial release we'll recommend that you don't enable data masking in Server Components until we've got full support in our streaming packages.

@jerelmiller
Copy link
Member Author

Hey all! I’m excited to announce that we’ve released Apollo Client 3.12.0-alpha.0 which contains the first experimental release of data masking 🎉 🎈 🎉 !

After installing the alpha version, you can enable data masking by setting the dataMasking option to true:

new ApolloClient({
  dataMasking: true,
})

This first alpha release does not contain everything that will be available in the 3.12 release. We have some known issues and a few remaining items left before we call this feature complete, mostly around tooling support for this feature (e.g. such as the codemod that adds @unmask to your queries for migration.) The core of the feature throughout the client however is implemented and part of alpha.0. We wanted to release something now so that you can begin trying it out and providing feedback!

I want to highlight a few important things for those that are looking to try this out, some of which are minor changes from the original proposal in this RFC.

@unmask

When initially proposed, @unmask was provided as a migration-only tool. As such, you would specify the directive on the query to unmask all named fragments in the query. We’ve changed this and instead require that you specify it with the fragment spread. We believe the additional granularity will provide an easier migration path to allow you to migrate a single subtree of fields at a time rather than the whole query.

Additionally, after much experimentation, we are instead making this a core directive that can be used as an escape hatch for operations that need access to the full result. We will actively discourage the use of this directive, but will be there in situations where using a masked type would otherwise prove difficult to use.

We still like the idea of leveraging this directive for a migration path to full adoption, so we’re giving you the ability to specify it as such with the mode argument set to "migrate".

query {
  user {
    id
    ...UserFields @unmask(mode: "migrate")
  }
}

Using the directive in this manner will enable warnings in the console when accessing the unmasked field instead of reading it through useFragment. This is to make it obvious which fields will disappear once masking is fully enabled by removing the @unmask directive.

TypeScript

We are going all-in on using the structure from the TypeScript types generated by GraphQL Codegen’s fragment masking feature. Its format provided enough flexibility for us to be able to construct both the masked and unmasked type definitions for any given operation.

Having access to a non-masked type is important for Apollo Client for a few reasons. First, we recognize that the path to adoption will more than likely be incremental. Giving you the ability to enable masking on a single operation type is important toward this path. Second, we have users that create multiple instances of Apollo Client for use in applications, typically with microfrontends. If you’re looking to enable data masking for a single one of these clients, it’s important that you can selectively determine which operations should be masked to avoid enabling to all at once. Lastly, some of the APIs, such as useMutation, provide the full result in callbacks intended for cache writes. The type definition needs to reflect this as well.

While we are adopting GraphQL Codegen’s fragment masking format, you don’t need to be using the client preset to use it. This functionality is already built into the typescript-operations plugin by setting the inlineFragmentTypes option to "mask". In fact, we will ask that you disable fragment masking use this feature. Apollo Client ships with all the types that would otherwise be generated from the fragment masking feature, so these extra types are not needed.

Important

Working with these types as of the time of this writing has one caveat. GraphQL Codegen does not yet understand the @unmask directive so types generated will not include fields marked with @unmask. We are working with The Guild to provide this capability in GraphQL Codegen and hope to have this released sometime soon.

Once you enable type masking with inlineFragmentTypes: "mask", you can start using these types right away, even if you haven’t yet enabled the dataMasking option in the client. By default, Apollo Client will unwrap that masked type into the full result type until you mark an operation as masked. You can do so a few different ways:

  1. Globally.

We've taken advantage of TypeScript's ability to do declaration merging. You can enabled globally masked types with Apollo Client using the following:

declare module "@apollo/client" {
  interface DataMasking {
    enabled: true
  }
}

NOTE: This is still experimental! Please try this out and let us know if you have troubles getting this to work.

  1. MaskedDocumentNode

If you’re a fan of TypedDocumentNode, you can tell Apollo Client that you'd like to use the masked type definition by switching to MaskedDocumentNode. This is a shortcut for using TypedDocumentNode<Masked<Query>, Variables> (more on this in the next point)

import { MaskedDocumentNode } from "@apollo/client";

const query: MaskedDocumentNode<MyQuery, MyQueryVariables> = gql`
  # …
`

Now you can use this query with any query API and data will be correctly typed using the masked type.

  1. Masked

You can mark an individual query type as masked using the Masked type helper. This is useful in situations where you might not have access to TypedDocumentNode, or where you prefer to pass generics to functions themselves. For example, to use this with useQuery by using its generic arguments instead of type inference:

import { Masked } from "@apollo/client";

const { data } = useQuery<Masked<MyQuery>, MyQueryVariables>()

Where its available

Masking is enabled for all operation types. This includes mutations, subscriptions and fragments. data returned by these operations be masked.

On the flip-side, cache APIs will not mask the returned data and instead give you the full result. We typically see direct cache reads paired with cache writes. If data returned from cache reads were masked, cache writes might suddenly become significantly more difficult since that data is "incomplete".

This extends to callbacks and options a part of APIs that would otherwise be masked. For example, useMutation has an update callback that allows you to perform a cache update once the mutation finishes. This callback receives the mutation result as an argument. This data is not masked to make it easier to perform your cache write.

Suspenseful useFragment

This is not yet part of the release. We are in the midst of experimenting with this and hope we will have something ready for the initial 3.12 release. I'll provide updates as I have them.

Misc

There has been some question on how exactly the from argument on useFragment will change as a result of this feature. For this initial release, we are focused on cached, normalized objects and as such, we've made no changes to this option. useFragment will continue to work exactly as it does today. Providing updates to non-normalized types has proven tricky so we are tabling this for now. If you pass a non-normalized object to from, you will see a warning in the console asking you to use the parent object instead.

This extends to no-cache queries as well. We are aware we need an answer for this so don't expect no-cache queries to work with useFragment. We are still trying to determine the best course of action for these types of queries. More to come in future updates.


Phew that was a lot. I hope this provides enough of a primer to get started!

We are very excited to be moving this forward and can't wait to get it in your hands. Please try this out and let us know what kinds of feedback you have, including any bugs you discover or edge cases we missed! Thanks!

@jerelmiller
Copy link
Member Author

Hey all 👋

We have just released 3.12.0-rc.2 which includes some bug fixes for data masking that we discovered while test driving this feature. Please try this version out and give us feedback! Unless we get reports of new bugs or discover more ourselves, the plan is to make this our public release version.

I'm excited to get this into your hands! Now onto writing documentation 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants