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

Refine Context.access modes to better convey intent #1016

Closed
foundrytom opened this issue Jul 12, 2023 · 4 comments
Closed

Refine Context.access modes to better convey intent #1016

foundrytom opened this issue Jul 12, 2023 · 4 comments
Assignees

Comments

@foundrytom
Copy link
Collaborator

foundrytom commented Jul 12, 2023

What

Refine the Context.Access enum to better convey the intent of the host.

Why

The current options leave ambiguity in the handling of publishing operations (see notes).

Acceptance Criteria

  • Updatet the Context.Access modes:
    • See proposal below for details of new modes
    • isForWrite method update to include the new kCreateRelated
    • IsMultiple removed
  • Clear documentation of when to use each mode are provided in the OpenAssetIO documentation.
    • Ie: use Write when you have a 1:1 reference, use CreateRelated when you are programmatically creating entities against a reference to another entity. Note that this means after preflight, as you then have a specific working ref, you a host should use kWrite for the ensuing register. If you're not calling preflight, then kCreateRelated should be used still.
    • Elaborate managementPolicyDocs
    • apiComplianceSuite coverage (spike to determine exact checks - eg, if you dont provide the fixtures, is there a default check for error cases, checks that preflight and register don't return the input ref when its not from preflight)

Notes

The behavior when publishing is currently defined solely in terms of how the published entities trait set relates to that of the target reference:

  1. When the reference points to a non-existant entity, that entity should be created.
  2. When the reference points to an existing entity:
    a. When the trait set of the existing entity matches that of the new entity, a new version should be created if allowed, or error.
    b. When the trait set of the existing entity does not match, the new entity as a child or other meaningful relation to the existing entity if permitted, or error.

The fact that most references are provided by the manager (or the manager's user) means that in many situations, this works well, and the intended result is easily discerned.

There are however edge cases that can arise with certain trait sets, or when more programatic operations are undertaken.

Making a "folder in a folder".

The common file system operation of making a folder inside a folder highlights the case for us in terms that are perhaps more accessible than OpenAssetIO abstract terminology.

As a host, if I wanted to make a folder, inside a folder for which I know the entity reference, using the publishing rules of thumb, I "publish the new folder to its parent", which follows 2b above. There is an issue however, the trait set of the new folder is the same as the parent (as it's a 'folder in a folder'). This means that the manager will assume we are intending 2a. It has now way of knowing that this is meant to be a new folder, rather than an update to the one targeted by the entity reference.

We need a way to tell the manager that the entities being published are always intended to be new.

The access property of the context exists to inform the manager of the intent of the host, so perhaps we already have a solution. If we look at the existing context access modes, one potential solution is kWriteMultiple as that implicitly suggests that "more are to come", so a manager could infer that this must be a creation operation. However, if a host is only making a single new entity, then it is quite illogical to set the access to something with "multiple" on the end.

If we further scrutinize that page, it becomes apparent, that half of the existing modes are only used in a UI context. Perhaps we need to re-think our options there.

@foundrytom
Copy link
Collaborator Author

foundrytom commented Jul 12, 2023

Proposal

Re work the Context.Access modes as follows:

  • Remove the kMultiple options
  • Add kCreateRelated (isForWrite true)

Removing kMultiple

These were previously used to inform a browser that multiple references could be simultaneously selected (kReadMultiple) or that a "container" must be selected (kWriteMultiple). There is no coherent use for these within the core API methods themselves, and so this behavior could be more accurately and coherently expressed as part of the UI layer itself. Avoiding polluting the core.

In addition, the use of "multiple" is quite confusing in a batch-first API. For example what is Write vs WriteMultiple in a batch context where you are already providing multiple references? These terms originated in the prior API form where there was no batching, so the multiple qualification was useful in suggesting there was "more to come". Though this is still the case, as there is no reason batch size is always aligned with number of entities to publish, the meaning is obscured by the form of the API signature.

kCreateRelated

It turns out, that in real-world workflows, there are many times, when the host doesn't actually know whether the resulting state is intended to be 1, 2a or 2b. This also turns out that this is not actually a problem. If the host workflow is such that it has a specific entity reference for each entity it is publishing, then it doesn't actually matter which scenario it is as it is always publishing to the corresponding reference.

The root cause of the problem as described is when a host has a single anchor entity reference, that it explicitly wishes to use to create new thing(s) in relation to. In the folder-in-a-folder example, the "new folder" button explicitly determines that this is never an update, it is always a new entity in relation to the current folder. Another example of this is available in #1003:

  • The render bundle is a 1:1 mapping of a known entity reference to the bundle, the existing logic is fine.
  • The layers, and aovs however, have no explicit entity references known to the host, it has to derive them using the '2b' approach above. Preflighting a layer to the bundle defines a new layer in the bundle and the manager must derive an entity reference for it, Preflighting an aov to one of these new layers then defines a new aov for the layer. The layers and aovs are always new entities from the host's perspective as it never had an explicit reference for these in advance.

The proposed kCreateRelated constant allows the host to express that this is the case in advance. It is a specialisation of kWrite defining that from the hosts perspective the publish should not be an update to an existing entity.
Resulting in the following behaviour definition for publishing:

  1. When the reference points to a non-existant entity, that entity should be created.
  2. When the reference points to an existing entity:
    a. When the access is kCreateRelated , A new entity is created in relation to the target entity where logical (eg: a child), or error.
    b. When the access is kWrite:
    i. When the trait set of the existing entity matches that of the new entity, a new version should be created if allowed, or error.
    ii. When the trait set of the existing entity does not match, behave as per 2a (we could say this should error if we wanted to be stricter, it would preclude a pipeline opting in to wanting to work like this).

Result

  • kRead: Use whenever API calls are in service of the consumption of data
  • kWrite: Use whenever API calls are in service of the creation of data against an entity reference that has been obtained for that specific target (ie, a 1:1 relationship between the reference and what is being created).
  • kCreateRelated : Use whenever API calls are in service of the creation of new entities related to a the entities targeted by user/manager supplied entity reference. Ie, the host is using some reference it already has, vs obtaining one specifically for the write.

@elliotcmorris
Copy link
Contributor

elliotcmorris commented Aug 8, 2023

If we wanted to extend BAL to test the register behaviour, we'd want a publish workflow where :

When registering against an existing entity ref

  • When kWrite is passed and the traitsets don't match, create a new entity and a relationship to that entity.
  • When kWrite is passed and the traitsets match, create a new version of that entity
  • when kCreateRelated is passed, irrespective of if the traitsets match, create a new entity and a relationship to that entity.

By giving BAL this behaviour, we can test kCreateRelated by passing kCreateRelated when the traitsets match, as asserting a new entity has been created, rather than a new version as would normally be done (with kWrite)

@elliotcmorris
Copy link
Contributor

elliotcmorris commented Aug 8, 2023

Documenting discussions with @feltech

This ticket has a problem in testing, specifically because of the open nature of the workflow, we're struggling to bring any specific api-compliance checks to bear.

There is a decent one we could do if we link the idea of supporting kCreateRelated with the neccesity of supporting getRelatedReferences, as they you can ask for a fixture with a relationship, and assert that a kCreateRelated register creates and entity of that relationship by calling getRelatedReferences. However, we're not convinced that link is valid.

So, to move forward, we're planning to test this as a BAL business logic suite, with the idea that we may be able to bring some tests over to api compliance if they make sense, sort of on a when-we see them basis. But even if not it serves as legitimate test coverage for this workflow.
To do this we'll need to do some work on BAL, but it's work we've been wanting to do anyway.

First, we want to give BAL the ability to write in both ways in OpenAssetIO/OpenAssetIO-Manager-BAL#62, doing the traitset-match check as we've described in the workflow outlines here. This can be done independently of this ticket.

Then, we'll need to do the retarget-workflow to give BAL the ability to understand kCreateRelated, (ie, retarget it against the openAssetIO branch with the new enum in it,) and write business logic tests, to ensure that the workflows work and make sense. Then we'll merge the OpenAssetIO enum and the BAL change together. That's the current thinking to move forward.

@foundrytom
Copy link
Collaborator Author

foundrytom commented Aug 9, 2023

(Sorry catching up, so sorry if I've misinterpreted anything)

Then we'll merge the OpenAssetIO enum and the BAL change together.

Can we focus on getting the changes in OpenAssetIO done and merged, and move on to tackle the other preflight changes. before you get onto BAL For this topic. As you noted, the fact that this test in in the business logic suite, highlights that this isn't something we need an integration test for to merge the OpenAssetIO changes, as they are not testing the implementation in the core library, but the fact that "you can conceptually implement this in some manager".

feltech added a commit to feltech/OpenAssetIO-Manager-BAL that referenced this issue Aug 21, 2023
Closes OpenAssetIO#57. OpenAssetIO/OpenAssetIO#1016 refined the Context access
patterns to formalize the creation of "children" and other related
entities via publish to an existing reference. BAL does not yet support
this kind of creation, and so should error in the case.

So respond appropriately to Access mode in various API functions,
by calling the provided error callback, except in the case of
`managementPolicy` which responds with empty `TraitsData` objects
(signifying "unmanaged").

Note that for consistency, this changes the error message output when
attempting to `resolve` with `kWrite` access.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO-Manager-BAL that referenced this issue Aug 21, 2023
Closes OpenAssetIO#57. OpenAssetIO/OpenAssetIO#1016 refined the Context access
patterns to formalize the creation of "children" and other related
entities via publish to an existing reference. BAL does not yet support
this kind of creation, and so should error in the case.

So respond appropriately to Access mode in various API functions,
by calling the provided error callback, except in the case of
`managementPolicy` which responds with empty `TraitsData` objects
(signifying "unmanaged").

Note that for consistency, this changes the error message output when
attempting to `resolve` with `kWrite` access.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO-Manager-BAL that referenced this issue Aug 21, 2023
Closes OpenAssetIO#57. OpenAssetIO/OpenAssetIO#1016 refined the Context access
patterns to formalize the creation of "children" and other related
entities via publish to an existing reference. BAL does not yet support
this kind of creation, and so should error in the case.

So respond appropriately to Access mode in various API functions,
by calling the provided error callback, except in the case of
`managementPolicy` which responds with empty `TraitsData` objects
(signifying "unmanaged").

Note that for consistency, this changes the error message output when
attempting to `resolve` with `kWrite` access.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO-Manager-BAL that referenced this issue Aug 23, 2023
Closes OpenAssetIO#57. OpenAssetIO/OpenAssetIO#1016 refined the Context access
patterns to formalize the creation of "children" and other related
entities via publish to an existing reference. BAL does not yet support
this kind of creation, and so should error in the case.

So respond appropriately to Access mode in various API functions,
by calling the provided error callback, except in the case of
`managementPolicy` which responds with empty `TraitsData` objects
(signifying "unmanaged").

Note that for consistency, this changes the error message output when
attempting to `resolve` with `kWrite` access.

Signed-off-by: David Feltell <david.feltell@foundry.com>
feltech added a commit to feltech/OpenAssetIO-Manager-BAL that referenced this issue Aug 23, 2023
Closes OpenAssetIO#57. OpenAssetIO/OpenAssetIO#1016 refined the Context access
patterns to formalize the creation of "children" and other related
entities via publish to an existing reference. BAL does not yet support
this kind of creation, and so should error in the case.

So respond appropriately to Access mode in various API functions,
by calling the provided error callback, except in the case of
`managementPolicy` which responds with empty `TraitsData` objects
(signifying "unmanaged").

Note that for consistency, this changes the error message output when
attempting to `resolve` with `kWrite` access.

Signed-off-by: David Feltell <david.feltell@foundry.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

2 participants