Jk i651 entity types cleanup

[JustKuzya]

Added the consistent uniform way to handle entity-types in the code. Rebased to the dev as of Aug 19, resolved all merge conflicts.

[keithmanville]

If the only two representations of a resource are the db name and the display name, why not use a standard Enum with db name as the the name and the display name as the value?

Does not address API routes. I thought this was one of the constants we would want to include.

[keithmanville]

remove debug prints before merging

[keithmanville]

remove these "separator" comments. Not used anywhere else in codebase.

[keithmanville]

why not just

return EntityTypes.to_snake_case(self.db_schema_name)

[JustKuzya]

Missed from the initial iteration.

[keithmanville]

variable names are confusing. We set db_schema_name to original_name and then the get_original_name() returns the db_schema_name. Are we trying to make some distinction between what we call this internally/externally from the enum?

[keithmanville]

I think in some cases were are translating to and from string representations when it's not needed. In this case, _resource_type ultimately comes from the calls to generate_resource_tags_endpoint where we pass RESOURCE_TYPE.get_db_schema_name().

We could just store the enum object directly and not need to go back and forth from string representations.

[jkglasbrenner]

I agree, and perhaps the way to do this is to do something like this in the ResourceIdService:

    @inject
    def __init__(self, resource_type: EntityTypes):
        self._resource_type = resource_type

And then the ResourceTagsIdService would get this modification:

    @inject
    def __init__(
        self,
        resource_type: EntityTypes,
        resource_id_service: ClassAssistedBuilder["ResourceIdService"],
        tag_id_service: TagIdService,
    ) -> None:
        self._resource_id_service = resource_id_service.build(
            resource_type=resource_type
        )
        self._tag_id_service = tag_id_service

And so on.

Of course, you'll also need to find where the resource_type argument is actually passed in each service layer definition and change that to the appropriate enum.

[hbooth]

agree, with storing the enum reference as much as possible and only output to string when needed.

[keithmanville]

I'm seeing several entries that aren't Resources. I thought the intent was to capture only Resources.

[jkglasbrenner]

This is also what I thought. There's a mixture of REST API entities, database tables that contain attributes about a resource type that is a backend implementation detail (the artifact parameters, for example), request parameters like PASSWORD, and HAS_DRAFT which is a derived attribute that we attach to resources in the app's responses.

[JustKuzya]

Until the end I was not sure if the third or fourth member would be needed for Errors and/or Logs. Neither was needed, but tuples would be easier to expand if I missed some use-cases later on.

[hbooth]

I think we need to make a strong distinction between REST Resources and internal Entity Types which generally map to a base object. If there are other ways or object that were referred to as an entity type, we need to resolve that. @JustKuzya let me know if you see others and let's discuss.

[keithmanville]

My review mostly posed questions, but it might be more helpful make a recommendation and see what everyone thinks:

• Make EntityTypes include only Resources
• Add the API routes to the EntityTypes enum
• Reduce reliance on get_from_string -- I think in most places we can store the enum directly. The place I'm thinking this doesn't work is where we have a user provided resource type as a string like in drafts.

I think those three would address my biggest questions with this PR.

[keithmanville]

I think wherever we use EntityTypes now it's cleaner to

from dioptra.restapi.v1.entity_types import EntityTypes

and then access the EntityTypes needed as EntityTypes.ARTIFACT.
Basically we don't need the extra ARTIFACT_RESOURCE_TYPE constants floating around anymore.

[hbooth]

agree with this.

[keithmanville]

remove getters and access properties directly.

[jkglasbrenner]

To that end, if you want to make these properties "read-only" (well, as much as you can do that in Python), prefer this pattern

    def __new__(
        cls,
        original_name: str,
        readable_name: str,
    ):
        """Default constructor for stuffing EntityTypes Enum

        Args:
            original_name (str): DB-schema-safe entity type name
            readable_name (str): The name to be read by user in UI/UX/Logs/Errors

        Returns:
            _type_: The created const-style immutable tuple-member the enum
        """
        new_instance = object.__new__(cls)
        new_instance._value_ = (
            original_name  # Set default value to make sure the Enum base works
        )
        new_instance._db_schema_name = original_name
        new_instance._ui_print_name = readable_name
        return new_instance

    @property
    def db_schema_name(self) -> str:
        return EntityTypes.to_snake_case(self._db_schema_name)

    @property
    def ui_print_name(self) -> str:
        return EntityTypes.to_snake_case(self._ui_print_name)

You would also need to remove these from the class definition to make room for the above:

    db_schema_name: str  # DB-compatible name representation
    ui_print_name: str  # Yser-friendly name representation

[jkglasbrenner]

My review focused on the scope of where EntityTypes should (and shouldn't) be used, some implementation details, and code organization. I also left comments on the threads started by @keithmanville when I had something to add.

[jkglasbrenner]

This kind of monkeypatch seems brittle and unpredictable, as it only is applied once this file is imported. Depending on how code is imported and called, it's very possible for json.dump to use different versions of the JSONEncoder default during the lifecycle of the application. The original default also is never restored, so this is a permanent override.

The usual way to do this kind of thing is to inherit from the JSONEncoder class, override the default method, and then pass it to json.dump or json.dumps explicitly:

import json
from enum import Enum


class EnumJSONEncoder(json.JSONEncoder):
    def default(self, obj):
        if isinstance(obj, Enum):
            return obj.name

        # Dispatch bask to the original mechanism of json-dumping for non-Enum objects
        return super().default(obj)


class Color(Enum):
    RED = 1
    GREEN = 2
    BLUE = 3


print(json.dumps({"favorite_color": Color.BLUE, "name": "First", "id": 1}, cls=EnumJSONEncoder))

which prints

{"favorite_color": "BLUE", "name": "First", "id": 1}

Stepping back, where is json.dump or json.dumps being used that requires this approach? Is it an invocation of json.dump or json.dumps that we control in the codebase? Or is it being invoked by one of our dependent libraries, and there's no way to provide the custom encoder?

If we truly need to override this for the entire application like this, then we should consider doing something similar to dioptra.restapi.patches.monkey_patch_flask_restx, which is called within dioptra.restapi.app.create_app. This would better control where in the application lifecycle the behavior gets changed.

[JustKuzya]

I like the instance switch suggested, but I don't remember what was breaking without the patch. I agree that probably a uniform way of doing it once and for all enums is better way to handle the Enums serialization.

[jkglasbrenner]

I consider this to be a general utility function instead of a method that is specific to the details of this class. I recommend extracting this into dioptra.restapi.utils, it would fit nicely right below the slugify function.

Also, after moving the function, I would move the regular expressions into module-level level constants and compile them, since in principle this function could be called many times.

CAMEL_PASCAL_SEP_REGEX = re.compile(r"(?<!^)(?=[A-Z])")
MULTI_SPACE_REGEX = re.compile(r"\s+")

# Used like
CAMEL_PASCAL_SEP_REGEX.sub(" ", text_to_snake)

[JustKuzya]

Done. Really good thought. Except for one thing: I always prefer to use a common prefix, in this care REGEX_... for similar purpose constants - this way the code assist becomes more helpful as all similar use constants have same prefix and pop up as a suggestion, developer only needs to read the suggestions carefully and choose. I know that most of the developers are "writers", not "readers" ☺ , but still it is more helpful than not for the most cases.

[jkglasbrenner]

To that end, if you want to make these properties "read-only" (well, as much as you can do that in Python), prefer this pattern

    def __new__(
        cls,
        original_name: str,
        readable_name: str,
    ):
        """Default constructor for stuffing EntityTypes Enum

        Args:
            original_name (str): DB-schema-safe entity type name
            readable_name (str): The name to be read by user in UI/UX/Logs/Errors

        Returns:
            _type_: The created const-style immutable tuple-member the enum
        """
        new_instance = object.__new__(cls)
        new_instance._value_ = (
            original_name  # Set default value to make sure the Enum base works
        )
        new_instance._db_schema_name = original_name
        new_instance._ui_print_name = readable_name
        return new_instance

    @property
    def db_schema_name(self) -> str:
        return EntityTypes.to_snake_case(self._db_schema_name)

    @property
    def ui_print_name(self) -> str:
        return EntityTypes.to_snake_case(self._ui_print_name)

You would also need to remove these from the class definition to make room for the above:

    db_schema_name: str  # DB-compatible name representation
    ui_print_name: str  # Yser-friendly name representation

[jkglasbrenner]

This is also what I thought. There's a mixture of REST API entities, database tables that contain attributes about a resource type that is a backend implementation detail (the artifact parameters, for example), request parameters like PASSWORD, and HAS_DRAFT which is a derived attribute that we attach to resources in the app's responses.

[jkglasbrenner]

Why not just use one of these everywhere instead of aliasing the two variants? I know there's inconsistency in naming in different sections of the code base (entrypoint is found in the Flask code, while entry_point is used in the SQLAlchemy ORM code), but will that impact which of these get used?

[JustKuzya]

The DB names seem to be set in stone (or rather "Philosophers Stone") of SQLAlchemy. Meanwhile the Textual representation is supposed to be human readable

[hbooth]

the whole idea of the entity was to allow a "translation" between the entity type as it is in the DB, the names it references, and the human readable description. Having an Enum also will likely prove useful for the repository pattern allowing a Repository instance to have a reference to the Entity that it is providing.

[jkglasbrenner]

I don't think I agree with changing this error to use an enum like this. Query parameters aren't entities, they're input data that's attached to a request sent to the endpoint.

[hbooth]

agree with this. The context is the REST resource, not an entity resource.

[jkglasbrenner]

Another place I'm not sure I would agree we should use EntityTypes. The sorting parameters map to attributes/properties on an Entity (or resource), they are not entities themselves.

[hbooth]

The type was passed in as a string value to provide context. I am not sure we should have two ways of providing this context. Do you have a suggestion how we can provide the proper context to indicate the properties that are (attempted) for sorting?

[hbooth]

after thinking about this a little bit, perhaps the better way to think about this, as not so much mapping to an entity but to a resource, with resource being the actual REST API Path we are mapping to. Therefore the mapping is not to an entity, but to the resource path.

[jkglasbrenner]

I don't agree that EntityTypes belongs here, see my other comment about query parameters.

[hbooth]

Instead of EntityType, this should be the REST resource on which this query parameter exists.

[jkglasbrenner]

I agree, and perhaps the way to do this is to do something like this in the ResourceIdService:

    @inject
    def __init__(self, resource_type: EntityTypes):
        self._resource_type = resource_type

And then the ResourceTagsIdService would get this modification:

    @inject
    def __init__(
        self,
        resource_type: EntityTypes,
        resource_id_service: ClassAssistedBuilder["ResourceIdService"],
        tag_id_service: TagIdService,
    ) -> None:
        self._resource_id_service = resource_id_service.build(
            resource_type=resource_type
        )
        self._tag_id_service = tag_id_service

And so on.

Of course, you'll also need to find where the resource_type argument is actually passed in each service layer definition and change that to the appropriate enum.