Skip to content

Closes: #11507 - Show Prefixes Aggregate and RIR on API view #18935

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

Closed
wants to merge 5 commits into from

Conversation

DanSheps
Copy link
Member

Closes: #11507 - Show Prefixes Aggregate and RIR on API view

  • Add rir property method to model
  • Add aggregate property method to model
  • Add rir attribute to serializer
  • Add aggregate attribute to serializer
  • Add test for both rir and aggregate fields on API

* Add `rir` property method to model
* Add `aggregate` property method to model
* Add `rir` attribute to serializer
* Add `aggregate` attribute to serializer
* Add test for both rir and aggregate fields on API
@DanSheps DanSheps added this to the v4.3 milestone Mar 18, 2025
@DanSheps DanSheps self-assigned this Mar 18, 2025
@DanSheps DanSheps marked this pull request as draft March 18, 2025 02:54
@DanSheps
Copy link
Member Author

Thinking about this, I think there are a few questions to ask before this can be reviewed:

  • Which is the better way to go: method property vs SerializerMethodField?
  • Would this benefit from being a cached field on the prefix model?

@DanSheps DanSheps linked an issue Mar 18, 2025 that may be closed by this pull request
@DanSheps
Copy link
Member Author

DanSheps commented Mar 27, 2025

Based on https://blog.bmispelon.rocks/articles/2024/2024-05-09-django-getting-a-full-model-instance-from-a-subquery.html

Could not figure out a way to get the RIR as part of the aggregate query, so I had to do them separately.

@DanSheps DanSheps requested review from a team and bctiemann and removed request for a team April 6, 2025 01:49
@DanSheps DanSheps marked this pull request as ready for review April 6, 2025 01:50
@jeremystretch jeremystretch requested review from jeremystretch and removed request for bctiemann April 7, 2025 13:24
Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

While innovative, I'm worried the solution involving a custom JSONModelField might be taking us too far into the deep end. The need for a custom model field feels like a very strong signal that we're going about this in the wrong way. This comment especially feels ominous:

Return the actual instantiated model from the fields, minus the models that cannot be worked with

We can pretty easily obtain the PK of the parent aggregate (if any) for each prefix using a subquery, which is half the battle. We just need a way to then manually prefetch the related Aggregate objects and stitch them together in a manner similar to how prefetch_related() works natively for ForeignKeys.

Have you looked into using using Prefetch objects at all?

@DanSheps
Copy link
Member Author

DanSheps commented Apr 8, 2025

While innovative, I'm worried the solution involving a custom JSONModelField might be taking us too far into the deep end. The need for a custom model field feels like a very strong signal that we're going about this in the wrong way. This comment especially feels ominous:

I don't disagree here. I personally think the better way to handle this would be through a foreign key relationship.

We can pretty easily obtain the PK of the parent aggregate (if any) for each prefix using a subquery, which is half the battle. We just need a way to then manually prefetch the related Aggregate objects and stitch them together in a manner similar to how prefetch_related() works natively for ForeignKeys.

I didn't look into using prefetch_related specifically but I did look into a way to aggregate/join that object and there does not appear to be an easy way to do that (outside of this here). From my understanding, both prefetch_related or Prefetch() require the use through a foreign key only. You cannot fetch unrelated models directly or through an aggregate. The issue is we are trying to return the full model and there really isn't a mechanism to do that outside of the foreign key relationship.

The issue is there is no FK relationship and you can't get another object outside of those FK relationships (with the exception of this) outside of a separate query. I think if anything, we would need a specific Relationship on the model which would allow us to enumerate, prefetch and join on these psuedo-relationships similar. I think this might be more work/effort then just simply adding in the proper relationship in the end.

@jeremystretch
Copy link
Member

I'm not seeing a ton of demand for this FR. Maybe it would be best to punt on it until we finally tackle #7845. I appreciate all the work you've put in to make this work @DanSheps, but I get the feeling it's likely to bite us in the ass in the future.

@DanSheps
Copy link
Member Author

DanSheps commented Apr 9, 2025

I am fine with that.

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.

Show Aggregate and RIR on Prefixes in API
2 participants