Rename existingItem argument to item for access control functions

[JedWatson]

cc/ @timleslie @molomby @mitchellhamilton

We talked not too long ago about whether we should change any of the arguments to access control functions and hooks for the new interfaces, and decided not to, in favour of reducing the upgrade complexity.

Since then, we've introduced new features to control things like fieldMode, which is designed to work alongside access control for better UX in the Admin UI. These take the new session argument and an item argument.

These functions aren't exactly the same as what you'd write for access control, but very similar, so I want to propose the change in this PR -- to rename existingItem to item in the access control arguments (but not hooks)

Rationale includes:

• The access function signature is changing, unavoidably, because of the new session argument and because we can't reliably provide a truly backwards-compatible authentication argument (and I'd argue that for this area of the system, because it's all about security, close enough is not good enough)
• existingItem doesn't really make sense in access control -- for functions where there is an item to provide, it always exists, it's just the item. Access control always happens at the same part of the lifecycle so there's no ambiguity about what it is, unlike with hooks
• I think this was previously named to align with hooks, and while I still think existingItem not the best name there, that's a whole other long conversation and I recognise the ambiguity involved in functions that are invoked in different parts of the operation lifecycle so I don't want to reopen that
• I believe it's now much more important for access control to align conceptually with other functions that control the availability of fields, than to align conceptually with hooks

This really stood out to me as a problem while I was working on the new auth example, but I wanted to submit this as a discrete PR after that one landed so you can see specifically the change I want to make.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d6f7996

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[jesstelford]

Any reason we couldn't retain existingItem for backwards compat, but keep it undocumented?

Eg:

-  const existingItem = ....;
+  const item = ....;
 
   return {
-    existingItem,
+    // Deprecated in favour of `item`
+    existingItem: item,
+    item,
   }

[JedWatson]

@jesstelford we (@mitchellhamilton and I) talked about that -- we could even mark it as deprecated in the TypeScript types, which does a good job of making it clear in the editor for TypeScript users.

However. Access control functions already need to be updated. Taking an example from our access control guide:

access: ({ authentication, existingItem }) =>
  authentication.item.isAdmin || authentication.item.id === existingItem.id

This already needs to be rewritten as:

access: ({ session, existingItem }) =>
  session.data.isAdmin || session.itemId === existingItem.id

At which point it might as well become:

access: ({ session, item }) =>
  session.data.isAdmin || session.itemId === item.id

Particularly since you'd want to also add a new function to control views in the Admin UI, which will look like this:

fieldMode: ({ session, item }) =>
  session.data.isAdmin || session.itemId === item.id ? 'edit' : 'hidden'

So the specific breaking change I'm proposing here doesn't have much impact on the refactor burden that already exists (for good reasons).

And if we leave the duplicate (undocumented/deprecated even) argument around:

1. When would we actually commit to removing it? If at any later date, we'd be foisting a second breaking change on everyone, when the easiest time to update is now
2. I think it's almost dangerously confusing. Unless you've read the docs, continuing to see existingItem (undocumented) as a newcomer to a codebase would be confusing if you were seeing item in other (newer) places. What's the difference between an item and an existingItem? They sound different... why are both provided? etc.

I think the second in particular has the potential to harm usability and clarity in this part of the system more I'm comfortable with, especially given the important of correctness and clarity when it comes to access control implementations.

Finally, as a little bonus, aligning the arguments for access and viewMode (and similar) functions has a nice effect that it would be simpler to write sugary abstractions over. An example of how this might realistically be implemented:

const isAdminOrItem = ({ session, item }) =>
  session.data.isAdmin || session.itemId === item.id;

///

access: isAdminOrItem,
fieldMode: args => isAdminOrItem(args) ? 'edit' : 'hidden',

Note that the arguments for both access and fieldMode aren't exactly the same, but we're expecting that it will be rare those differences will be used (i.e they're there to support more edge cases, from what we can tell) so I expect implementations like the one immediately above will be quite common.

[jesstelford]

This already needs to be rewritten as:

Why can't we do the same aliasing for session here too?

When would we actually commit to removing it? If at any later date, we'd be foisting a second breaking change on everyone, when the easiest time to update is now

For those who can update now, sure, this makes sense. But for those who can't, it becomes a non-starter for upgrading. Instead of forcing extra work on them, we can be lenient and give them a grace period (I've often seen strategies of deprecation such as X months, or Y major versions).

As someone who will want to upgrade to get access to other nice new things, I don't also want that upgrade to force me into having to go over all my custom access control (and hooks?) to update them, all for the sake of us adding 1 line to Keystone.

[timleslie]

So I think there's probably a few different things to consider here:

1. In a vacuum, are we happy we happy (in a colour-of-the-bikeshed sense) with the name change existingItem to item? I'm 👍 on that and I think that's been pretty reasonably justified.
2. What is the impact for existing projects? This won't break existing projects, but it will be a thing that needs to be changed in the future, so it's a question of impact when changing to the new APIs. More on this below.
3. What are the impacts on the design/dev of the new APIs? Ideally we want to keep these as clean and simple as possible in order to make them easy to document and reason with. From that POV this is 👍
4. What's the path look like between now and when we make the new APIs available for GA? We're still a way off this, so we have time to hone in on the right solution before we get established projects upgrading to these APIs. If it turns out in a week or three that we're wrong on this we're still in no-harm/no-foul territory.

From the point of view of 1) 3) and 4) I think we should go ahead and make this change now, since this doesn't change anything in existing keystone, so there's no danger there.

From the point of view of 2), I think this will be a comparatively low-impact change relative to other things that need to be changed in order to switch to the new APIs for an existing project. In particular, I suspect there are very few instances where existingItem is used, but authentication is not, so the switch from authentication to session will be a bigger change to be dealt with.

This bigger question of how we make this transition as smooth as possible for pre-existing projects is something we need to be constantly keeping in mind as we move through this phase. In this instance there's probably a few different options on the table. We could ship it as is and get people to make the changes themselves, we could ship a wrapper that automatically translates the arguments, we could provide deprecated aliases as per Jess' suggestion above, and probably some other things that don't come to mind right now.

Each of these options have tradeoffs. I think we should hold off making in a decision until we've had a chance to do some experiments with the new packages against pre-existing projects and we can see where the pain points are for transitioning. Adding backwards-compatibility helpers is totally a thing we will want to do, and adding these helpers in the future will be easier than removing them, so I think for today we should ship this as is and then revisit this discussion on backwards compatibility helpers and transition paths once we have a bit more experience with the impacts.

[JedWatson]

Solid writeup @timleslie, thanks!

With that in mind, I'm going to merge this, lets revisit those open points once the new interfaces have been in people's hands for a bit but before they go GA 👍

[jesstelford]

I like the words @timleslie said 👍

[JedWatson]

He does have a way 😀