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.

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

[RFC] Querying and serializer interfaces for ResourceInstance and TileModel #11595 #11596

Draft
wants to merge 73 commits into
base: dev/8.0.x
Choose a base branch
from

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Oct 31, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

This first iteration has TODOs enumerated and will need some tests, but this is functional enough to invite some testing and feedback. See #11595 for motivation. I'll update the bottom of this PR with DX questions that come up as I polish.

The basic idea:

  • use the Django ORM's annotate() feature for joining tile data to resource instance queries
  • memoize some information about how the data was fetched to handle saving it back
  • delegate to the datatype classes how to perform tile queries or convert tile values to python instances
    • e.g. we can return real model instances for ListItemValues or related ResourceInstances if we wish
  • provide serializers

Testing instructions

  • See example project-level serializers: Prototype concept and scheme serializers #103 arches-lingo#104
    • includes setup instructions and links to the browsable APIs for django rest framework APIs. These serializers can receive static JSON data.
  • See example queries in querysets.py.
    • I have ideas about adding more features to make the queries a little more ergonomic.

Issues Solved

Closes #11595

Checklist

  • I targeted one of these branches:
    • dev/8.0.x (under development): features, bugfixes not covered below
    • dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
    • dev/6.2.x (extended support): major security issues, data loss issues
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Ticket Background

  • Sponsored by: Getty Conservation Institute
  • Designed by: @jacobtylerwalls in conversation with many :-)

Further comments

TODO: flesh out DX questions below

Comment on lines +77 to +80
def find_root_node(prefetched_siblings, nodegroup_id):
for sibling_node in prefetched_siblings:
if sibling_node.pk == nodegroup_id:
return sibling_node
Copy link
Member Author

@jacobtylerwalls jacobtylerwalls Oct 31, 2024

Choose a reason for hiding this comment

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

It just occurred to me could add a column is_root_node to the Node model, perhaps as a GeneratedField so that we don't have to run these searches.

if tile_val is not NOT_PROVIDED:
datatype_instance = datatype_factory.get_instance(node.datatype)
python_val = datatype_instance.to_python(tile_val)
setattr(tile, node.alias, python_val)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is pretty well hidden (I'll declutter and add some signposting), but here is where I'm attaching the tile value to the tile object by node alias. Similar place in the ResourceInstanceQuerySet.

If we need to "seal" these python values so that we can know when they've been "tampered" with to make computation of saving back a little faster, I'm imagining we can wrap this in some sort of class with custom __setattr__ and __getattr__ methods to set that dirty state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Er, probably just on the __getattr__, and treat anything accessed as potentially dirty, because you can directly mutate things without setting something on the python object directly (unless we consider that a feature, that such an update is blocked...)

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.

Interface for querying & updating resource models by node & nodegroup aliases
1 participant