Skip to content

Conversation

@mraerino
Copy link
Contributor

@mraerino mraerino commented Jan 21, 2025

Fixes: #17709

The logic for passing a primary key when specifying a nested model on creation or update is hidden in some special logic in BaseModelSerializer and cannot be detected by drf-spectacular.

I'm introducing an extension that patches the write request schemas to allow for either the primary key as a number or an object with fields (what was already present).

This aligns the schema with reality.

@mraerino mraerino changed the title Fix request OpenAPI schemas for nested models Allow PK for nested models in OpenAPI request schemas Jan 22, 2025
@mraerino mraerino marked this pull request as ready for review January 22, 2025 12:54
@mraerino mraerino changed the title Allow PK for nested models in OpenAPI request schemas Allow primary key for nested models in OpenAPI request schemas Jan 22, 2025
@mraerino
Copy link
Contributor Author

mraerino commented Feb 2, 2025

@jeremystretch any chance i could get a review here?

@hikhvar
Copy link

hikhvar commented Feb 14, 2025

Any updates on this? I generated a Go client with these changes and it really looks very good.

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

Just the one simple optimization otherwise looks good.

@mraerino mraerino force-pushed the fix-openapi-request-pk branch from e97dbe4 to 7216a0d Compare February 18, 2025 18:53
@arthanson
Copy link
Collaborator

arthanson commented Feb 28, 2025

@mraerino sorry one more minor, there is a merge conflict, can you please fix that. As this code is from a separate repository I can't make the quick fix without creating a new PR.

re the syntax change for the walrus operator (:=) not really required - just more concise, in this case can be debatable if it is more readable or not. Generally we use that in newer code (since it is a newer python addition) where applicable, but it was a suggestion so if you feel it is cleaner without I'm okay with that.

Also, can you please request re-review (upper right spinner next to name) when you have made the change so it pops up in my queue, makes it easier to track all the incoming PRs.

@hikhvar
Copy link

hikhvar commented Mar 3, 2025

@arthanson I only know this syntax from Go, and there an assignment within the condition of an if statement is only valid within that if/else block. We use schema later in line 296 not in the scope of the if block. Is this covered by the walrus operator? Just to understand the scoping here.

@mraerino
Copy link
Contributor Author

mraerino commented Mar 4, 2025

@mraerino sorry one more minor, there is a merge conflict, can you please fix that. As this code is from a separate repository I can't make the quick fix without creating a new PR.

I merged main in to fix the conflict. (Funnily it was me who introduced the other change that lead to the conflict.)

Did you know that all people with access to this repo are allowed to push to my branch since i checked "Allow edits by maintainers" on this PR?

it was a suggestion so if you feel it is cleaner without I'm okay with that.

I'd rather keep the code like it is to avoid confusion about variable scope as pointed out by @hikhvar

@mraerino mraerino requested a review from arthanson March 4, 2025 20:33
@arthanson arthanson merged commit 631ff3e into netbox-community:main Mar 5, 2025
3 checks passed
@bctiemann bctiemann mentioned this pull request Mar 5, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2025
@mraerino mraerino deleted the fix-openapi-request-pk branch June 1, 2025 11:57
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.

OpenAPI Specification Does Not Allow Numeric IDs for Related Objects, Contrary to Documentation

3 participants