-
Notifications
You must be signed in to change notification settings - Fork 21
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
IFC-1245 Initial implementation for object templates #5610
Conversation
CodSpeed Performance ReportMerging #5610 will not alter performanceComparing Summary
|
18008c2
to
f036a13
Compare
ef6cf14
to
954f385
Compare
8ec5689
to
9833cd0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some minor comments for now, will look more later.
@@ -313,6 +314,7 @@ def to_int(self) -> int: | |||
"Lineage", | |||
"Schema", | |||
"Profile", | |||
"Template", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add this to the release notes to indicate that we're adding a new namespace to the list of restricted ones.
@@ -265,7 +272,7 @@ async def query( | |||
async def count( | |||
cls, | |||
db: InfrahubDatabase, | |||
schema: Union[type[SchemaProtocol], NodeSchema, GenericSchema, ProfileSchema, str], | |||
schema: Union[type[SchemaProtocol], NodeSchema, GenericSchema, ProfileSchema, TemplateSchema, str], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking here but for the count and query methods when we are updating them we might as well get rid of the Union
.
@@ -327,13 +341,22 @@ def get_profile(self, name: str, duplicate: bool = True) -> ProfileSchema: | |||
raise ValueError(f"{name!r} is not of type ProfileSchema") | |||
return item | |||
|
|||
def get_template(self, name: str, duplicate: bool = True) -> TemplateSchema: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be kind
instead of name
? Same for SchemaBranch.get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the same parameter name as all the methods that perform the same kind of operations. I agree that name
is not the best name for it, I used it for consistency reasons. I guess we could rename that parameter for all the methods in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Looks good to me overall. I left some comments, the most important one would be related to the scenario if we can't find the template during the processing of a node.
@@ -546,7 +547,7 @@ def generate_graphql_object(self, schema: MainSchemaTypes, populate_cache: bool | |||
|
|||
interfaces: set[type[InfrahubObject]] = set() | |||
|
|||
if isinstance(schema, NodeSchema | ProfileSchema) and schema.inherit_from: | |||
if isinstance(schema, NodeSchema | ProfileSchema | TemplateSchema) and schema.inherit_from: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should have a common name for NodeSchema | ProfileSchema | TemplateSchema
in the same we we have for MainSchemaTypes
.
|
||
await obj_peer.new(db=db, **obj_peer_data) | ||
await node_constraint_runner.check(node=obj_peer, field_filters=list(obj_peer_data)) | ||
await obj_peer.save(db=db) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When saving the peer here we would have a changelog for that peer under obj_peer.node_changelog
I think we'd want the .handle_relationships() method to send back a list[NodeChangelog]
to the caller in order to get a full picture of what has changed.
@@ -197,13 +282,14 @@ async def mutate_create_object( | |||
await obj.new(db=db, **data) | |||
await node_constraint_runner.check(node=obj, field_filters=fields_to_validate) | |||
await obj.save(db=db) | |||
await cls._handle_object_template(db=db, branch=branch, obj=obj, data=data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you do something like this instead
object_template = await cls.get_object_template(db=db, branch=branch, obj=obj, data=data)
if object_template:
await cls._handle_template_relationships(
db=db,
branch=branch,
constraint_runner=node_constraint_runner,
template=object_template,
obj=obj
)
seems like _handle_object_template
is basically getting an object template and then running it through _handle_relationships
and _handle_relationships
doesn't need to get its own version of NodeConstraintRunner
this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That node constraint runner setup bugs me as well. When I tried to use the existing one, built by the mutation code, it failed at identifying the original node (Unable to find the node X / KIND
error). My guess is that it does not play very well with a performance fix we pushed earlier this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving this some thoughts I wonder if it is going to be an issue with relationships allocated from pool, if they are also used in uniqueness constraints.
ece08c3
to
aadb959
Compare
e96db49
to
50a6eaf
Compare
e2846fb
to
f9aa039
Compare
This is the initial implementation of object templates.
An object template is a schema node that is generated on the fly when a node defines
generate_template: true
. When generating object template schema, the schema manager will traverse parent/component relationships in order to determine which templates are required.Nodes for which templates are generated will automatically get a
object_template
relationship pointing to the template that was used to create the objects. Templates will also have arelated_nodes
relationship with references to objects created by templates.Templates have their own namespace and are published in the REST API schema under the
templates
key.When running a GraphQL mutation, if an
object_template
relationship is specified, attributes from the template will be applied as the ones of the object to create. Attributes with values given in the mutation input will still prevail over the ones of the template.When creating an object, parent/component relationships are resolved and objects will also be created. This is done in 2 steps: it first creates the initial object which is based on the given template, then it goes through relationships and populates them.
Relationships of other kinds are not yet supported but can be implemented at the next step of this feature implementation.