Skip to content
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.

Sign up for GitHub

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

Jump to bottom

birger/512 uri #1463

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

birger/512 uri #1463

wants to merge 6 commits into from

Conversation

b1rger
Copy link
Contributor

@b1rger b1rger commented Nov 29, 2024 •
edited
Loading

  • feat(apis_metainfo): add a GenericForeignKey to the Uri model
  • feat(apis_entities): update AbstractEntity to work with generic uri
  • feat(apis_relations): update GenericTripleForm to work with generic uri
  • feat(generic): update Enrich view to work with generic uri
  • feat(utils): update create_object_from_uri to work with generic uri

Closes: #512

@b1rger b1rger force-pushed the birger/512-uri branch 2 times, most recently from 2946903 to ed318dc Compare November 29, 2024 09:31
@b1rger b1rger marked this pull request as ready for review November 29, 2024 09:34
@b1rger b1rger added the needs-attention This issue or pull request is in need of discussion, information, assessment by team members label Dec 2, 2024
@sennierer sennierer removed the needs-attention This issue or pull request is in need of discussion, information, assessment by team members label Dec 3, 2024
@b1rger b1rger force-pushed the birger/512-uri branch 4 times, most recently from 283842b to a7f35fd Compare December 4, 2024 11:12
@b1rger b1rger requested a review from sennierer December 17, 2024 09:38
@b1rger b1rger force-pushed the birger/512-uri branch 5 times, most recently from 02a60bd to 41d99dd Compare December 17, 2024 12:53
@b1rger b1rger requested a review from gythaogg December 20, 2024 07:16
gythaogg
gythaogg reviewed Dec 20, 2024
View reviewed changes
Copy link
Contributor

@gythaogg gythaogg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Data migration ✔️
Import via external URI ✔️

It seems like this PR removes the feature that allows user to link a Uri to a RootObject
image Is that tabled for another PR or are we going to do away with manual linking of entities with URIs altogether and only rely on Merge and Import views to add an external Uri to an Object?

And likewise, it is possible to link Uri to any object, but at the moment this is also not supported via the webapp, right?

I was able to create Uris and link to non RootObject objects like Profession and a Relation via django shell

image

We should decide if it's worth having a clean up signal that clears the generic FK fields when the linked object is deleted. At the moment the object shows up as None but the content type lingers. In the database both generic FK fields hold the invalidated data.

image

And the "view" page of a Uri shows less information than the list view (related entity and entity type are missing). Not saying that it should be part of this PR but it is also a downgrade - as the related object (Root object ID) used to be part of the page.

Regarding apis_relations: Could we have Property Class URI as a Uri object linked to a property?

I created a apis_relations properties like so:
image

And I tried to link the following entities with "located within" relation
image

But I got this error
image

It is possible that this error is nothing to do with this PR but I was not able to test this PR owing to this error - I was also not able to setup the "located within" property with subject and object types set to the RootObject. Got 'NoneType' object has no attribute '__subclasses__' error.

@b1rger b1rger force-pushed the birger/512-uri branch 2 times, most recently from a5fe1c6 to 2066511 Compare December 20, 2024 10:47
@b1rger
Copy link
Contributor Author

b1rger commented Dec 20, 2024

Data migration works ✔️

thats the most important part ;)

It seems like this PR removes the feature that allows user to link a Uri to a RootObject
Is that tabled for another PR or are we going to do away with manual linking of entities with URIs altogether and only rely on Merge and Import views to add an external Uri to an Object?

good point. I had added editable=False to the fields, but to keep the previous behavior it makes more sense to keep them editable. the edit and create forms now allow to set the content_type and object id.

And likewise, it is possible to link Uri to any object, but at the moment this is also not supported via the webapp, right?

thats now possible

@gythaogg
Copy link
Contributor

We should decide if it's worth having a clean up signal that clears the generic FK fields when the linked object is deleted. At the moment the object shows up as None but the content type lingers. In the database both generic FK fields hold the invalidated data.

Another point: default Uri created for an object should be deleted if the related object is deleted, we need a signal for this I think.

Regarding apis_relations: Could we have Property Class URI as a Uri object linked to a property?

Not the point of this PR, moving it to a new issue once this is merged.

It is possible that this error is nothing to do with this PR but I was not able to test this PR owing to this error - I was also not able to setup the "located within" property with subject and object types set to the RootObject. Got 'NoneType' object has no attribute '__subclasses__' error.

Confirmed by @b1rger This is an existing error, irrelevant to current PR

It seems like this PR removes the feature that allows user to link a Uri to a RootObject
Is that tabled for another PR or are we going to do away with manual linking of entities with URIs altogether and only rely on Merge and Import views to add an external Uri to an Object?

good point. I had added editable=False to the fields, but to keep the previous behavior it makes more sense to keep them editable. the edit and create forms now allow to set the content_type and object id.

Tested, works ✔️

And likewise, it is possible to link Uri to any object, but at the moment this is also not supported via the webapp, right?
thats now possible

Works ✔️

@b1rger
feat(apis_metainfo): replace Uri.root_object with GenericForeignKey
f56dba2 
The `Uri` should allow to point to any model instance, not only to models
that inherit from `RootObject`. Therefore we introduce a
GenericForeignKey. The migration copies the values of existing
`root_object` pointers to the generic foreign key. The `root_object`
field is then dropped from the model.
@b1rger
feat(apis_entities): update create_default_uri to use generic uri
0856e92 
The Uri model now uses a generic foreign key instead of the direct
foreign key to the RootObject. This commit updates all occurences
of `root_object` to use the generic foreign key instead.
@b1rger
feat(apis_relations): update GenericTripleForm to work with generic uri
e33e708 
The `Uri` model now uses a generic foreign key instead of the direct
foreign key to the `RootObject`. This commit updates all occurences
of `root_object` to instead use the generic foreign key.
@b1rger
feat(generic): update Enrichview & GenericModel to work with generic uri
ab26065 
The `Uri` model now uses a generic foreign key instead of the direct
foreign key to the `RootObject`. This commit updates all occurences of
`root_object` to use the generic foreign key instead.
@b1rger
feat(utils): update create_object_from_uri to work with generic uri
6e8d1a3 
The `Uri` model now uses a generic foreign key instead of the direct
foreign key to the `RootObject`. This commit updates all occurences of
`root_object` to use the generic foreign key instead.
@b1rger b1rger requested a review from gythaogg December 20, 2024 14:45
gythaogg
gythaogg requested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@gythaogg gythaogg left a comment

Choose a reason for hiding this comment

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

Everything works as expected and the migrations are seamless.

Only one unresolved point about orphan URIs when the linked object is deleted. In the current implementation if a URI is linked to an object which is then deleted then the URI gets deleted too. I think we should preserve this behaviour at least for the default URI that's created using APIS_BASE_URI or the default example.org

Other external URIs that are manually created and then linked to an object or added to the database via Import/Enrich features - I think should either be deleted like the default URIs as described above or have their generic FK fields set to None.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gythaogg gythaogg gythaogg requested changes

@sennierer sennierer Awaiting requested review from sennierer

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make URI more generic
3 participants
@b1rger @gythaogg @sennierer