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

Replace convention of created_by being None for system process #92

Open
fenekku opened this issue Dec 1, 2021 · 1 comment
Open

Replace convention of created_by being None for system process #92

fenekku opened this issue Dec 1, 2021 · 1 comment

Comments

@fenekku
Copy link
Contributor

fenekku commented Dec 1, 2021

There was a convention of using None in the created_by field to mark the creator as the system process. This should be changed to just another regular entity representation for this identity e.g. {"system": -1} or some such thing. This will greatly reduce the None checking overhead and uniformize the code base.

This change may have to actually be made in a module higher up.

@max-moser
Copy link
Contributor

A scan through RDM-Records, Requests, and Records-Resources (the latter two after the lift-up refactoring) gave me the following results:

RDM-Records

  • The list of record owners (record.parent.access.owners) does not accept None as a value; .add(None) will raise a TypeError: invalid owner type: <class 'NoneType'>.
  • The services.components.access:AccessComponent is responsible for initializing the list of owners for a new record based on the identity used to call service.create(...); if it is the system_identity, the list of owners will just stay empty (no None value).
    Otherwise, it will use a dictionary of the shape {"user": identity.id}.
  • The services.generators.RecordOwners generator will interpret every owner as user (i.e. it will create a UserNeed for each entry in the list of owners).

None of the spots mentioned above actually use None (instead of a proper ref_dict) to represent the system.
Here, it's more a question of whether or not it makes sense for records to have an empty list of owners, i.e. no owners at all (which I think is still acceptable).
However, we should probably discuss which entities are eligible to be a record's owner (only users? maybe communities? ...), and then rework these spots using the newly added mechanisms for entity references (and restrictions on the referenced entity types).

Records-Resources

The Resolver Registry encodes the system_identity as None in reference_entity(...).
This seems to be the only place where None is used with a meaning other than "there is no reference".

Requests

While requests must have some routing information (a sender and a receiver), it is currently entirely possible for requests to not have a topic (i.e. topic = None).
I can imagine some types of requests that do not have a specific topic in the future (e.g. very general support requests that aren't about a record specifically), so I think this will still hold true then.

Conclusion

I think that it should still be valid to have a value of None instead of a proper reference dictionary in some cases.
However, the semantics should shift from "None represents the system" to "None means there is no reference".
This means that we cannot fully eliminate all None checks, but at least get a much better picture of when and where None could be expected and what it means.
For representing the system, we should come up with an actual and explicit dictionary.

Examples: {"system": 0} (here, the value doesn't actually matter), {"special": "system"}, ...

Also, we need to introduce a new Entity Proxy, as well as decide on how to .resolve() this entity.

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

No branches or pull requests

2 participants