Skip to content

Conversation

@carlos-villavicencio-adsk
Copy link
Contributor

This pull request refactors the _get_type_and_id_from_value function to improve how entity dictionaries are processed. The function now extracts a broader set of allowed keys, not just type and id, making the output more informative and flexible.

@carlos-villavicencio-adsk carlos-villavicencio-adsk requested a review from a team November 6, 2025 22:16
@carlos-villavicencio-adsk carlos-villavicencio-adsk marked this pull request as draft November 7, 2025 17:35
@carlos-villavicencio-adsk carlos-villavicencio-adsk marked this pull request as ready for review November 10, 2025 21:56
Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case showing the exact problem that we are trying to fix here, please?

I recently worked with local storage path (relative path and all) and I don't understand the problem (nor I experience it with my own tests).

Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THanks for the update. One more minor comment.

Also, can you update the test case please to include a local storage update example?

Copy link
Contributor

@julien-lang julien-lang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have a few more comments

elif isinstance(field_value, list):
new_value = []
for fv in field_value:
if isinstance(fv, dict):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add and "id" in field_value and "type" in field_valuehere too?

Copy link
Contributor

@julien-lang julien-lang Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or call recursivity:

def _optimize_filter_field(field_value: Union[dict, list], recursive: bool =True) -> Union[dict, list]:
    """
    For an FPT entity, returns a new dictionary with only the type,
    id, and other allowed keys.
    If case of any processing error, the original dictionary is returned.

    At least `type` and `id` keys are required to do the optimization
    """
    allowed_keys = {
        "id",
        "type",
        "url",
        "name",
        "content_type",
        "local_path",
        "storage",
        "relative_path",
    }
    try:
        if (
            isinstance(field_value, dict)
            and "id" in field_value
            and "type" in field_value
        ):
            return {key: field_value[key] for key in allowed_keys if key in field_value}

        elif recursive and isinstance(field_value, list):
            return [_optimize_filter_field(fv, recursive=False) for fv in field_value]

    except (KeyError, TypeError):
        LOG.debug(f"Could not optimize entity value {field_value}")

    return field_value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I;ve added id and type for now. But I'll take a look the implications about using recursion here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Also, one more crazy idea: can we get rid of try/except (KeyError, TypeError)? I mean at this point in the code, which exception can we expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion. Making it recursive slightly improved performance by a few hundredths of a second.

I took this chance to add more and more tests cases to validate more scenarios.

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

Successfully merging this pull request may close these issues.

4 participants