[WIP] Consistent crud #8827
[WIP] Consistent crud #8827
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
||
| @serializable() | ||
| @total_ordering | ||
| class NewDateTime(pydantic.BaseModel): |
There was a problem hiding this comment.
If the old Datetime was a syft object with a version then you can rename the class since the lookup is from the canonical name, which means you can call this one DateTime.
I do still think removing the lookup / versioning aspect of this type just means that we corner ourselves for future changes.
This could easily be fixed by adding another object to the .mro()
SyftObjectBase(pydantic.BaseModel, SyftHashableObject) <- no versioning
SyftObjectVersioned(SyftObjectBase) <- new class with canonical_name and version
SyftObject(SyftObjectVersioned) <- all the crazy new fields that are being added mostly for our larger Document store style objects, which I agree feels overkill for a DateTime object.
# the old class
class DateTimeV2(SyftObject):
__canonical_name__ = "DateTime"
__version__ = SYFT_OBJECT_VERSION_2
# your new class here
class DateTime(SyftObjectVersioned):
__canonical_name__ = "SyftObject"
__version__ = SYFT_OBJECT_VERSION_3
@koenvanderveen thoughts?
There was a problem hiding this comment.
This new interface would indeed fix this problem, but at the same time we are currently looking into how to properly handle third-party objects in serialization as in the migration PR we have started to use canonical names instead of fully qualified names. This could be a solution for versioning as well, by adding an exception for NewDateTime without it having to inherit from SyftObject
| _deleted_date: NewDateTime | None = None | ||
|
|
||
| # @property | ||
| def is_deleted(self) -> bool: |
There was a problem hiding this comment.
@koenvanderveen can you make sure that we address the actual permanent deletion of objects seperately. For example if there was some dangerous code or a very large blob we want to remove that should be possible. I figure the version upgrade surgery tools will be the best place to add this kind of behavior that allows for precise manual changes to data.
| return self.stash.object_type.__canonical_name__.lower() | ||
|
|
||
| def set(self, context: AuthedServiceContext, obj: Any) -> SyftError | SyftSuccess: | ||
| # if hasattr(obj, id): |
There was a problem hiding this comment.
How are our permissions being checked here. Are they admin only or do they allow any user? Do we check for owner, or write. Also are we allowing for the uid to be set by the user as we do in action objects (where we make sure its unclaimed first).
Many of the CreateModelType objects are uid free and generate one during the transform. I dont 100% think this matters too much either way as long as perform the permission checks, but if we do allow users to inject their own UIDs we actually need to make sure they are unique in the entire system (the action store as well) otherwise they could create an object with the same uid as an object in the action store and... do something weird probably.
It could be that the generic set is admin only and is only used by developers to write their own create object / methods in services although I imagine at least building a consistent pattern on top of set for everyone is a good idea, or creating a super().create() might also be helpful.
There was a problem hiding this comment.
The permissions are checked once when the API is created and once in the store, we could also add some safeguards there as well, though I think for some methods we already have that, but I am not sure how consistent they are. From my understanding, this PR is only for the services and we will refactor store as well sometime in the (hopefully near) future.
Regarding the UID issue, indeed you are right that it might be a problem, we can leave set for admin only and add create as well, with the assumption that we have CreateModelType object for every object. I could work on adding that if you agree.
Description
Please include a summary of the change, the motivation, and any additional context that will help others understand your PR. If it closes one or more open issues, please tag them as described here.
Affected Dependencies
List any dependencies that are required for this change.
How has this been tested?
Checklist