Add references support #8662

bartv · 2025-01-20T14:12:03Z

Description

Add references support (to the dataclasses branch)

wouterdb

First pass

wouterdb · 2025-01-20T14:20:50Z

src/inmanta/ast/type.py

+        if references.is_reference_of(value, float):
+            return True


This is the main open issue I think

Do I understand correctly that the issue is that we don't always want to allow references, so this is too accepting?

We could have a Reference(value_type=Type) type, who's validate checks exactly this. Then validate callers could always be explicit in whether they want to allow references? I'd have to double check which callers exist, but I expect that for each of them being explicit would be both feasible and more clear.

Or perhaps a validate_reference method on Type. Would have the advantage that we can rely on polymorphism for the Float -> float mapping etc. But it's less flexible in the sense that adding support for more special cases (Unknown etc) requires a special method for each.

wouterdb · 2025-01-20T14:22:44Z

src/inmanta/export.py


        for type_name, handler_definition in Commander.get_providers():
            code_manager.register_code(type_name, handler_definition)
+            types.add(type_name)
+
+        # Register all reference and mutator code to all resources. This is very coarse grained and can be optimized once


it register everything that is loaded, even if not in use. There is something to say for that, but it is very greedy.

wouterdb · 2025-01-20T14:24:49Z

src/inmanta/references.py

+class Argument(pydantic.BaseModel):
+    """Base class for reference (resolver) arguments"""
+
+    type: str
+    """ The type of the argument. It is used a discriminator to select the correct class
+    """
+
+    name: str
+    """ The name of the argument in the reference constructor
+    """
+
+    @abc.abstractmethod
+    def get_arg_value(self, resource: "inmanta.resources.Resource") -> object:
+        """Get the value for the argument to be able to construct the reference again
+
+        :param resource: The resource on which the reference has been defined
+        """
+
+
+class LiteralArgument(Argument):
+    """Literal argument to a reference"""
+
+    type: typing.Literal["literal"] = "literal"
+    value: PrimitiveTypes
+
+    def get_arg_value(self, resource: "inmanta.resources.Resource") -> object:
+        return self.value
+
+
+class ReferenceArgument(Argument):
+    """Use the value of another reference as an argument for the reference"""
+
+    type: typing.Literal["reference"] = "reference"
+    id: uuid.UUID
+
+    def get_arg_value(self, resource: "inmanta.resources.Resource") -> object:
+        return resource.get_reference_value(self.id)
+
+
+class GetArgument(Argument):
+    """Get a value from the resource body as value"""
+
+    type: typing.Literal["get"] = "get"
+    dict_path_expression: str
+
+    def get_arg_value(self, resource: "inmanta.resources.Resource") -> object:
+        return None
+
+
+class PythonTypeArgument(Argument):
+    """Use the python type as an argument. This is mostly used for references that are generic for a
+    type, such as core::AttributeReference
+    """
+
+    type: typing.Literal["python_type"] = "python_type"
+    value: str
+
+    def get_arg_value(self, resource: "inmanta.resources.Resource") -> object:
+        return getattr(builtins, self.value)
+
+
+class ResourceArgument(Argument):
+    """This argument provides the resource itself to the reference"""
+
+    type: typing.Literal["resource"] = "resource"
+
+    def get_arg_value(self, resource: "inmanta.resources.Resource") -> object:
+        return resource
+
+
+ArgumentTypes = typing.Annotated[
+    LiteralArgument | ReferenceArgument | GetArgument | PythonTypeArgument | ResourceArgument,
+    pydantic.Field(discriminator="type"),
+]
+""" A list of all specific types of arguments. Pydantic uses this to instantiate the correct argument class
+"""
+
+
+class BaseModel(pydantic.BaseModel):
+    """A base model class for references and mutators"""
+
+    type: ReferenceType
+    args: list[ArgumentTypes]
+
+
+class ReferenceModel(BaseModel):
+    """A reference"""
+
+    id: uuid.UUID
+
+
+class MutatorModel(BaseModel):
+    """A mutator"""
+
+
+C = typing.TypeVar("C", bound="Base")


As I understand, this defines the serialized form of the reference

wouterdb · 2025-01-20T14:29:08Z

src/inmanta/references.py

+    type: typing.ClassVar[ReferenceType]
+
+    def __init__(self, **kwargs: object) -> None:
+        self._arguments: typing.Mapping[str, object] = kwargs


I wonder if it wouldn't make more sense to e.g. pick up all values from __dict__ that don't start with _

I.e. this requires all arguments to be passed down, while this information is already available?

wouterdb · 2025-01-20T14:31:04Z

src/inmanta/references.py

+                    model = value.serialize()
+                    arguments.append(ReferenceArgument(name=name, id=model.id))
+
+                case "resource", _, inmanta.resources.Resource:


Does the third argument match in case of subtyping?

wouterdb · 2025-01-20T14:31:41Z

src/inmanta/references.py

+    def serialize(self) -> BaseModel:
+        """Serialize to be able to add them to a resource"""
+
+    def serialize_arguments(self) -> Tuple[uuid.UUID, list[ArgumentTypes]]:


This will need some more error reporting in a later stage I think

wouterdb · 2025-01-20T14:34:40Z

src/inmanta/references.py

+                case _:
+                    raise TypeError(f"Unable to serialize argument `{name}` of `{self}`")
+
+        data = json.dumps({"type": self.type, "args": arguments}, default=util.api_boundary_json_encoder, sort_keys=True)


may not be entirely stable, because the argument could be re-ordered when manipulating _arguments

wouterdb · 2025-01-20T14:35:44Z

src/inmanta/references.py

+        return self._model
+
+
+class DataclassRefeferenMeta(type):


Suggested change

class DataclassRefeferenMeta(type):

class DataclassReferenceMeta(type):

wouterdb · 2025-01-20T14:43:08Z

src/inmanta/resources.py

@@ -292,7 +386,12 @@ def object_to_id(

    @classmethod
    def map_field(
-        cls, exporter: Optional["export.Exporter"], entity_name: str, field_name: str, model_object: "proxy.DynamicProxy"
+        cls,
+        exporter: Optional["export.Exporter"],


I would expect the exporter object exposed to every export function to enabled the resource to register a reference even if it has no mutator (for plugins that have built-in, not transparent support)

wouterdb · 2025-01-20T14:44:24Z

src/inmanta/resources.py

+            self._references_model[model.id] = model
+
+        for mutator in self.mutators:  # type: ignore
+            mutator = references.mutator.get_class(mutator["type"]).deserialize(references.MutatorModel(**mutator), self)


We may want to define a base class resource to signal serialization failure , just to guide developer towards cleaner exceptions

sanderr

intermediate comments

sanderr · 2025-02-04T09:16:11Z

src/inmanta/const.py

+RESOURCE_ATTRIBUTE_REFERENCES: typing.Final[str] = "references"
+RESOURCE_ATTRIBUTE_MUTATORS: typing.Final[str] = "mutators"


These should not be exposed in the model, should they?

sanderr · 2025-02-04T09:25:24Z

src/inmanta/plugins.py

We'd need to check if references are allowed somewhere in the arg validation.

src/inmanta/ast/type.py

sanderr · 2025-02-04T09:37:56Z

src/inmanta/ast/type.py

+        if references.is_reference_of(value, float):
+            return True


Do I understand correctly that the issue is that we don't always want to allow references, so this is too accepting?

We could have a Reference(value_type=Type) type, who's validate checks exactly this. Then validate callers could always be explicit in whether they want to allow references? I'd have to double check which callers exist, but I expect that for each of them being explicit would be both feasible and more clear.

Or perhaps a validate_reference method on Type. Would have the advantage that we can rely on polymorphism for the Float -> float mapping etc. But it's less flexible in the sense that adding support for more special cases (Unknown etc) requires a special method for each.

sanderr · 2025-02-04T09:42:01Z

src/inmanta/execute/util.py

+    An instance of this class is used to indicate that this value is a reference and can only be resolved
+    at execution time in agent.


can only be resolved at execution time in agent.

Not entirely true iirc, though probably so in 90% of the use cases.

sanderr · 2025-02-04T09:53:16Z

src/inmanta/references.py

+# TODO: we need to make sure that the executor knows it should load the mutator and executor code before running the handler
+#       of a resource that uses references.


This is done now, isn't it?

sanderr · 2025-02-04T09:53:43Z

src/inmanta/references.py

This file probably needs some stable_api annotations

sanderr · 2025-02-04T09:54:00Z

src/inmanta/references.py

+
+
+class mutator:
+    """This decorator register a mutator under a specific name"""


This is not meant to be exposed to end users for now?

sanderr · 2025-02-04T09:55:14Z

src/inmanta/references.py

+        return type.__getattr__(cls)  # type: ignore
+
+
+class Reference[T: RefValue](Base):


If I understand correctly, the main interface starts here, and everything (except type vars etc) above this line is internals? Might be more readable to move this to the top, or move the internals to a submodule?

sanderr · 2025-02-04T09:59:12Z

src/inmanta/util/__init__.py

@@ -559,7 +559,7 @@ def _custom_json_encoder(o: object) -> Union[ReturnTypes, "JSONSerializable"]:

    from inmanta.data.model import BaseModel

-    if isinstance(o, BaseModel):
+    if isinstance(o, (BaseModel, pydantic.BaseModel)):


I'm not sure about this one. It may be safe, but I'd need to double check what exactly is custom about our own models. I think we may not be able to guarantee round-trip compatibility (or timezone-awareness) of native pydantic models.

sanderr

More comments.

Note to self: to review: references.py and resources.py.

sanderr · 2025-02-04T10:03:57Z

src/inmanta/util/dict_path.py

+        if not hasattr(container, "__getitem__"):
+            return False
+
+        if set and not hasattr(container, "__setitem__"):
+            return False
+
+        if remove and not hasattr(container, "__delitem__"):
+            return False


I'd prefer isinstance checks on the ABCs here, but iirc there' a separate ticket to implement this properly.

sanderr · 2025-02-04T10:42:18Z

src/inmanta/references.py

+                    model = value.serialize()
+                    arguments.append(ReferenceArgument(name=name, id=model.id))
+
+                case "resource", _, inmanta.resources.Resource:


If we special-case this, we should make sure to document that.

sanderr · 2025-02-04T10:46:54Z

src/inmanta/references.py

+        :param name: This name is used to indicate the type of the reference
+        """
+        self.name = name


I think this will need to be changed somewhat with what we discussed earlier today. i.e. we'll want to accept a Python type and convert it with to_dsl_type. Would be even nicer if we can derive the Python type from the type subscription (i.e. the value for the generic type var) of the Reference, but I'm not sure how feasible that would be tbh.

sanderr

LGTM overall, added some comments.

sanderr · 2025-02-06T13:03:43Z

src/inmanta/resources.py

+    :param path: The current path we are working on in the tree
+    """
+    match value:
+        case list() | proxy.SequenceProxy():


Not sure, but we might be able to replace this simply with

Suggested change

case list() | proxy.SequenceProxy():

case Sequence:

sanderr · 2025-02-06T13:09:23Z

src/inmanta/resources.py

+            const.RESOURCE_ATTRIBUTE_MUTATORS not in obj_map
+            or obj_map[const.RESOURCE_ATTRIBUTE_MUTATORS] is None
+            or const.RESOURCE_ATTRIBUTE_REFERENCES not in obj_map
+            or obj_map[const.RESOURCE_ATTRIBUTE_REFERENCES] is None


Suggested change

const.RESOURCE_ATTRIBUTE_MUTATORS not in obj_map

or obj_map[const.RESOURCE_ATTRIBUTE_MUTATORS] is None

or const.RESOURCE_ATTRIBUTE_REFERENCES not in obj_map

or obj_map[const.RESOURCE_ATTRIBUTE_REFERENCES] is None

obj_map.get(const.RESOURCE_ATTRIBUTE_MUTATORS) is None

or obj_map.get(const.RESOURCE_ATTRIBUTE_REFERENCES) is None

sanderr · 2025-02-06T13:15:55Z

src/inmanta/resources.py

+        case dict() | proxy.DictProxy():
+            return {key: collect_references(value_reference_collector, value, f"{path}.{key}") for key, value in value.items()}


If I understand correctly, we're recursing on data we don't control, right? i.e. the attribute as specified by the user. Doesn't that mean that we can't assume that key matches dictpaths invariants? e.g. what if it contains . or * or other characters with special meaning?

sanderr · 2025-02-06T13:17:23Z

src/inmanta/resources.py

+        self._add_reference(reference)
+
+        self.mutators.append(
+            references.ReplaceValue(


I've made my mind up, I like this approach.

sanderr · 2025-02-06T13:19:07Z

src/inmanta/resources.py

+            case list():
+                for v in value:
+                    self._add_reference(v)
+
+            case dict():
+                for k, v in value.values():
+                    self._add_reference(v)


Is this a leftover from a previous implementation? As far as I can tell, the way this method is called, it would always be a Reference.

sanderr · 2025-02-06T14:21:45Z

src/inmanta/references.py

+        self.reference = reference
+
+    def resolve(self) -> T:
+        return typing.cast(T, getattr(self.reference, self.attribute_name))


Since we transparently skip what would otherwise be model validation (e.g. when assigned to a resource's attribute, should we add some validation here?

bartv added 30 commits January 26, 2024 13:07

First version of value references

490c049

Phase 2

1a3927d

Linting

144bb6d

Merge remote-tracking branch 'origin/master' into references

d38fe92

Update

4c3dd3f

Merge remote-tracking branch 'origin/master' into references

706d843

Update

0d3a386

merge

42f4b20

Merge remote-tracking branch 'origin/master' into references

a81f950

First implementation

91c5ce2

Merge remote-tracking branch 'origin/master' into references

8cc9640

Revert plugins

79a5fa7

Fix a bug that made many tests fail

f175263

Add a comment

bdc3ba1

Remove a failing test

e0e5912

Merge remote-tracking branch 'origin/master' into references

3929315

Improve

fe0f2ef

Fix unkown handling

fefb255

Fix handling of json

80057bc

Reformat

8bd522e

Update

7cdae7d

Update

650bde5

Format

99daea7

Updates

946b963

Merge remote-tracking branch 'origin/master' into references

c942007

Update

48876ba

Merge remote-tracking branch 'origin/master' into references

4f525f5

Merge remote-tracking branch 'origin/master' into references

3abeb16

Update

b2c6d61

Linting

515946b

bartv added 21 commits December 5, 2024 11:54

Fixes

afe113d

Merge remote-tracking branch 'origin/dataclasses' into references

49f52e0

Update

0cb7f08

Merge remote-tracking branch 'origin/dataclasses' into references

543fa68

Support dataclasses

dbfa01b

Update

2343a57

Fix type errors

8a29158

Fix more type errors

6030235

Type fixes

84c7285

Update

f924d45

Merge branch 'dataclasses' into references

3ad61d9

Fix tests

05e9485

More fixes

a2da645

Add code loading

2a01cad

Merge remote-tracking branch 'origin/dataclasses' into references

3b12ee8

Fix imports

6e0dcbc

Update

95bdef1

Merge remote-tracking branch 'origin/dataclasses' into references

500ae46

Update

317e17b

Merge remote-tracking branch 'origin/dataclasses' into references

3da9916

Add test case in progress

8fa4f2c

bartv requested review from wouterdb and sanderr January 20, 2025 14:12

wouterdb reviewed Jan 20, 2025

View reviewed changes

sanderr reviewed Feb 4, 2025

View reviewed changes

sanderr reviewed Feb 6, 2025

View reviewed changes

inmantaci deleted the branch dataclasses February 6, 2025 15:32

inmantaci closed this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add references support #8662

Add references support #8662

bartv commented Jan 20, 2025

wouterdb left a comment

wouterdb Jan 20, 2025

sanderr Feb 4, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

wouterdb Jan 20, 2025

sanderr left a comment

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr left a comment

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr Feb 4, 2025

sanderr left a comment

sanderr Feb 6, 2025

sanderr Feb 6, 2025

sanderr Feb 6, 2025

sanderr Feb 6, 2025

sanderr Feb 6, 2025

sanderr Feb 6, 2025

	class DataclassRefeferenMeta(type):
	class DataclassReferenceMeta(type):

		RESOURCE_ATTRIBUTE_REFERENCES: typing.Final[str] = "references"
		RESOURCE_ATTRIBUTE_MUTATORS: typing.Final[str] = "mutators"

		An instance of this class is used to indicate that this value is a reference and can only be resolved
		at execution time in agent.

		# TODO: we need to make sure that the executor knows it should load the mutator and executor code before running the handler
		# of a resource that uses references.



		class mutator:
		"""This decorator register a mutator under a specific name"""

		return type.__getattr__(cls) # type: ignore


		class Reference[T: RefValue](Base):

		case dict() \| proxy.DictProxy():
		return {key: collect_references(value_reference_collector, value, f"{path}.{key}") for key, value in value.items()}

Add references support #8662

Add references support #8662

Conversation

bartv commented Jan 20, 2025

Description

wouterdb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment