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

Fixes: #14606 - For Cable API add PK for cable terminations to serializer #17201

Closed
wants to merge 5 commits into from

Conversation

DanSheps
Copy link
Member

Fixes: #14606

  • Add pk field for cable termination
  • Add test

@DanSheps DanSheps added the type: bug A confirmed report of unexpected behavior in the application label Aug 20, 2024
@DanSheps DanSheps self-assigned this Aug 20, 2024
@jeremystretch jeremystretch removed the type: bug A confirmed report of unexpected behavior in the application label Aug 23, 2024
@jeremystretch
Copy link
Member

This one is probably a bit stickier than I realized in the original bug report.

We should really be using a nested CableTerminationSerializer here, omitting its cable and cable_end fields (as they're redundant in this context). That would address the primary issue.

However, the catch is that the generic serializer currently in use returns the fields object_type, object_id and object, whereas CableTerminationSerializer returns termination_type, termination_id, and termination. Swapping out the serializer would be a breaking change.

We could address this in v4.1, however I'm hesitant to slip in a breaking change so late in the beta period. For now, we could settle with just adding the id field to GenericObjectSerializer, and capture this as a potential change for v4.2.

'TracedCableSerializer',
)


class GenericObjectTerminationSerializer(GenericObjectSerializer):
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't require a new serializer: We can just add the id field to GenericObjectSerializer.

Copy link
Member Author

@DanSheps DanSheps Aug 25, 2024

Choose a reason for hiding this comment

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

Unfortunately since a_terminations/b_terminations is a property function that includes the actual termination (Interface/CircuitTermination/etc) and not the CableTermination object it won't be the proper id (it will be the interface id for example, not the cable termination id).

This is why I opted to go this route, as as you mention, it would be a breaking change. Perhaps we should aim for 4.1 instead and I just rebase and swap the serializer.

It may also make more sense to maintain a_terminations/b_terminations as is (add ID however to GenericObjectSerializer still) and add a generic "terminations" which operates on Cable.terminations which is I think where it would be appropriate to use the CableTermination serializer.

netbox/dcim/tests/test_api.py Outdated Show resolved Hide resolved
@DanSheps DanSheps changed the base branch from develop to feature August 30, 2024 02:39
@DanSheps DanSheps added the breaking change This change modifies or removes some previously documented functionality label Aug 30, 2024
@DanSheps DanSheps added this to the v4.1 milestone Aug 30, 2024
@DanSheps
Copy link
Member Author

  • Rebased to feature
  • Modified serializer to add terminations related objects

Let me know if you would rather replace a_terminations / b_terminations (will be non-trivial as they currently only return the Terminated Objects (Interface, etc)

@jeremystretch jeremystretch removed the breaking change This change modifies or removes some previously documented functionality label Sep 2, 2024
@jeremystretch
Copy link
Member

This is going to be more involved than I originally anticipated, because the a_terminations and b_terminations attributes of a cable instance are populated with the actual terminating objects, rather than CableTermination instances. We don't want to just tack these on in addition to the existing attributes because that would be redundant and inefficient. We'll have to explore our options, which likely include tweaking the underlying querysets for the Cable model.

@jeremystretch jeremystretch removed this from the v4.1 milestone Sep 3, 2024
@jeremystretch
Copy link
Member

Appreciate your work on this @DanSheps. I'm going to close it out for now; let's re-evaluate our options for a potential change in v4.2.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminations listed for a cable in the REST API should include their numeric IDs
2 participants