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

Change REST API to use hyperlinks instead of raw PKs for related objects with depth 0 #3679

Merged
merged 7 commits into from
May 3, 2023

Conversation

glennmatthews
Copy link
Contributor

@glennmatthews glennmatthews commented May 2, 2023

Related to #3691

What's Changed

Change the base REST API serializer inheritance from ModelSerializer to HyperlinkedModelSerializer. This changes our rendering of related objects at depth 0 from raw PK UUIDs (which don't provide enough useful information, such as the content-type of the related object) to API URLs (which intrinsically show the type of the related object, and are also walkable by API clients to easily retrieve details of specific related objects if desired. Note that on the "write" side of the API, it's still possible to describe related objects by raw PK or by nested serializer data - this change only impacts the "read" side of the API.

This wasn't quite a drop-in replacement because for whatever reason, DRF's HyperlinkedModelSerializer, HyperlinkedRelatedField, and HyperlinkedIdentityField classes are not aware of URL namespacing, and so they default to trying to automatic reverse urls like <modelname>-detail instead of what we need of <applabel>-api:<modelname>-detail. I've overridden what I believe to be all of the places where this was being used incorrectly.

This lets us remove the explicit url field from most of our serializers as a bonus, because it's now provided automatically by the serializer now that we have correct logic in place for finding the URL for a given object instance.

image

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

@itdependsnetworks
Copy link
Contributor

Can you confirm/deny that the effect of this is that if as a plugin developer, I use BaseModelSerializer, it will automatically generate a field called "url" and it will correctly link.

@glennmatthews
Copy link
Contributor Author

Can you confirm/deny that the effect of this is that if as a plugin developer, I use BaseModelSerializer, it will automatically generate a field called "url" and it will correctly link.

That should be correct, yes.

@glennmatthews
Copy link
Contributor Author

To be clear that is not the primary intent of this PR, but it is a bonus. I'll add more details tomorrow.

@glennmatthews glennmatthews changed the title Change REST API to use HyperlinkedModelSerializer Change REST API to use hyperlinks instead of raw PKs for related objects with depth 0 May 3, 2023
@glennmatthews glennmatthews self-assigned this May 3, 2023
@glennmatthews glennmatthews marked this pull request as ready for review May 3, 2023 15:05
nautobot/core/tests/test_api.py Outdated Show resolved Hide resolved
Comment on lines +338 to +340
result = [entry.get_absolute_url(api=True) for entry in obj_related_field.all()]
if serializer.context.get("request"):
result = [serializer.context.get("request").build_absolute_uri(url) for url in result]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the distinction here with request vs no request? Is get_absolute_url here not enough to make the BrowableAPI walkable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for self-consistency. get_absolute_url alone doesn't know about the protocol/host/port info (it returns e.g. /api/dcim/devices/<uuid>), request.build_absolute_uri is what adds that information (http://localhost:3000/api/dcim/devices/<uuid>).

Copy link
Contributor

@jathanism jathanism left a comment

Choose a reason for hiding this comment

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

💋

@glennmatthews glennmatthews merged commit 3e9e8d4 into next-ui May 3, 2023
@glennmatthews glennmatthews deleted the u/glennmatthews-rest-api-hyperlinked branch May 3, 2023 19:05
@bryanculver bryanculver added the type: feature Introduction of new or enhanced functionality to the application label May 8, 2023
@bryanculver bryanculver added this to the v2.0.0 milestone May 8, 2023
bryanculver added a commit that referenced this pull request May 9, 2023
* detail view is able to render

* Make API serializers only emit JSON schema on OPTIONS queries

- Generic create view is now working againn and will correctly redirect on create
- `uiSchema` is now being enforced on Form compponents for create view
- API endpoints no longer include `serializer/formData` keys and is back to the default endpoint responses from v1
- `schema/uiSchema keys now emit on `OPTIONS` queries only
- This is currently a POC hack. The correct solution should replace the metadata class and not overload `finalize_response()` on the viewsets.
- A number of extra `FIXME` comments put into place circle back to for correctness.

* Add storybook container

* Run CI on commit of next-ui. Style and lint

* handle value rendering when it is a list and when it has a web url

* Prettier and lint

* Moved all JSON schema rendering code to `NautobotMetadata`

- Only the Metadata class emits JSON schema now when an `OPTIONS` request is made
- Legacy `NautobotMetadata` renamed to `NautobotMetadataV1` for now
- All other code that was in `nautobot.core.api.views` has been ripped out and there is no longer a need to overload or modify anything with REST API views
- Made `eslint` happy

* set redux cache timeout to 5 seconds

* fix button padding

* Added remaining field type mappings for JSON schema

- Caveat: `ImageField` of type "image" is not happy from `@rjsf/core` and reports it as "Unsupported schema type"; so it is hard-coded as "string" type for the moment.

* notes tab and changelog tab render correctly for some ...

* update list view title / buttons

* Less swr

* Nav menu revisions WIP (#3652)

* Restructure nav menu

* Change nav menu API return, update context when switching object list views, only show menu for current context

* Prettier and lint

* Get sub-group menus working

* ESLint

---------

Co-authored-by: Bryan Culver <me@bryanculver.com>

* Flatten components directory

* Fix paths from flattening

* Missed import, hide scroll

* Fix warning about changed weight

* Highlight active icon based on appContext

* Fix minor issue with breadcrumb menus

* Really fix weight issue

* ESLint

* Fix some tests

* Update some docstrings and behavior

* Add change fragment, update docs, remove invalid template

* correct rendering of list objects and added capabilities to render plugin tabs

* fix ObjectListTable Title

* minor fix

* format

* Enforce user permissions on GetMenuAPIView

* Remove stray console.log()

* Revise metadata to include fields from DRF stock metadata

- Refactored `NautobotProcessorMixin` to have `(ui)SchemaProcessor` inherit from it
- Added support for using "format" and "readOnly" schema attributes
- Got mapping for `ImageField` working to emit a file widget
- Moved custom serializer field type mappings to `settings.DRF_REACT_TEMPLATE_TYPE_MAP`
- Foreign keys are temporarily mapped to just a string of format "uuid" so that unit tests will pass
- Merged generic view tests for `test_options_objects_returns_display_and_value` and `test_options_returns_expected_choices` as they were pretty much doing the same thing and the `display`/`value` paradigm does not carry over in the JSON schema metadata.
  - Temp patch to '`validate_regex` validator to have a `message` attribute so tests will pass. It should be replaced with a `BaseValidator` subclass.
- UI form will now catch form submissions errors but does not yet update the UI with the errors
- Updated `black`/`flake8` configs to skip `node_modules` which can apparently have a ton of Python code in them we don't want linted

* Home view generic, retry

* Make Prettier happy.

* Implement `ObjectCreate` async form validation

* Restore vendored alias

* Initial mockup of Home view

* `ObjectCreate` form will always live validate and clear errors on change.

* DetailView mock API response

* Update List View (#3654)


Co-authored-by: Gary Snider <gary.snider@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>

* add NtcThumbnailIcon to detail view table headers

* Disable `liveValidate` on `ObjectCreate` form because it's annoying.

* Introduce HomePanel component to reduce repetition in Home view

* Add JobHistoryTable mock component

* Refine Home view

* Detail View Config API endpoint configured at detail_url/detail_view_config and detail view is now rendering based on the API JSON response

* added plugin tabs for demo purposes

* Enhance useGetRESTAPIQuery, put live data into some items in the Home view

* Test fixes

* update tests to be timezone agnostic

* Fix eslint for test

* More test fixes

* Stashing

* Docs updates

* Bug fix

* Remove dead code, update docs

* Integration test fixes

* replace slug urls with pks (WIP)

* Fix pylint

* Fix some slug tests

* Fix flake8, simplify menu definition slightly

* Remove redundant CI

* minor fixes for ObjectRetrieveView

* Implemented concept of "default" vs. "all" headers for table columins in list view.

Python
- In keeping with how Django's `ModelAdmin.list_display` controls the default columns displayed in object list view (table view), implemented a `ModelViewSet.list_display` attribute that is a list of the fields to display by default.
- Enhanced metadata class to include a `view_options` mapping of which `list_display` is an entry. Since these elements are NOT for form generation/presentation, they did not go into the `schema/uiSchema` objects.
  - The field names are emitted as a list of dicts in the same format that the JS table component expects headers to be provided (e.g. `[{"name": "foo", "label": "Foo"}, ...]`.

JS
- The props for `ObjectListTable` renamed/extended
  - Added `defaultHeaders` for the list of fields that will be displayed by default
  - Renamed `tableHeader` to `tableHeaders` to indicate it's plural
- All non-default fields will be selectable in the action menu in the top right of the table header
- State store is used so that column changes are persistent for a user's local environment

* Simplify get-menu API response and corresponding handling of it

* Pull update to nautobot-ui. Fix context update.

* Recursive breadcrumb construction

* fix legacy ui nav menu

* Add missing element

* Update menu to context inversion

* Overflow table

* Fixes for legacy nav menu

* Improved support for default fields to always keep them first.

- `list_display` translates to default table columns that will always be displayed first
- Metadata on an OPTIONS request now also includes `fields` inside of `view_options`
- All other fields will be displayed after those
- Updated `DateTimeField` schema to correctly emit a `date-time` widget.

* Move secrets to Security menu, change url for object-change records

* Add HomeChangeLogPanel for real data

* More legacy test fixes

* Add real job-result data to the Home view

* paginator loading...

* paginator limit argument working

* Update `invoke eslint` and `invoke prettier` to support local

- Also updated `.eslintignore` and `.prettierignor` to ignore `app_imports.js` and `jsconfig.paths.json`

* Make `validate_regex` into a real validator class.

* Fix integration tests

* Make storybook not run by default; add some docs for it

* Docstring update

* Allow linking of Status and Role objects in ObjectListTable

* Squashed commit of the following:

commit d94e50a
Author: Bryan Culver <bryan.culver@networktocode.com>
Date:   Mon May 1 10:10:39 2023 -0400

    Loading handling

commit 344f150
Author: Bryan Culver <bryan.culver@networktocode.com>
Date:   Mon May 1 10:04:04 2023 -0400

    Polished retrieve

commit 3805707
Author: Bryan Culver <bryan.culver@networktocode.com>
Date:   Fri Apr 28 21:43:05 2023 -0400

    stashing

* Fix prettier config

* Fix logic to handle plugin URLs.

* Handle enum values. Use em-dash instead of faMinus icon for null values

* add Pagination to ObjectListView

* Update installed-plugins REST API endpoint, re-add Installed Apps UI view

* Re-add conditional removed by merge

* Move created/last_updated to header of ObjectRetrieveView

* add API endpoint to quickly provide object counts for home view

* re-add slug to legacy job form

* fix home page row spacing

* PR feedback

* LoadingWidget and polishing up API endpoints for detail view notes and changelogs

* Serializer field ordering with `field_order` and `list_display` defined on the model.

- `list_display` is now defined on at `FooSerializer.Meta.list_display. If not defined all fields on the serializer will be used.
- `FooModelSerializer.Meta.fields` replacing `__all__` with explicit field ordering with the assertion that new views will also use new serializers, where the desired list of fields can be provided.
- Yet unsolved is how to account for what were previously "virtual" table fields such as `Prefix.children` (count) and `utilization`. These should probably be migrated to serializer opt-in fields.

* merge ObjectListTable and ObjectListTableNoButtons

* Remove vestigial `list_display` and `field_order` from `BaseModel`.

* Make `JobHistoryTable` a bit more error-tolerant

* fixed formatting and addressed feedback

* Fixed TableItemTests

* force redirect to login page

* remove commented out javascript code in legacy ui

* update home page security panel

* Skeleton for ObjectListView Loading

* fix react bugs

* Add some trailing slashes for consistency between ui and django

* revert some ObjectListView changes

* Change REST API to use hyperlinks instead of raw PKs for related objects with depth 0 (#3679)

* Change API to use HyperlinkedModelSerializer

* flake8 failure that crept in during merge

* pylint ftw

* Test fix and change fragments

* Update return_nested_serializer_data_based_on_depth to use URLs instead of PKs with depth 0

* Make an attempt at updating the REST API docs - definitely not comprehensive

* Address review feedback

* Remove web_url

* Fix redirect after object create

* more login improvements

* Fix missing breadcrumb on ObjectRetrieve

* Fix a few react complaints

* Remove redundant favicon file

* use title case on object list skeleton

* change GetMenuAPIView to only work for authenticated users

* enhanced the ObjectListView loading skeleton and fixed? ObjectListTable width

* final polish on detail view

* Implemented list_display_fields for IPAM serializers. (#3696)

* Implemented list_display_fields for IPAM serializers.
* Renamed `Meta.list_display` to `list_display_fields` for improved readability.
* Fixed a bug in UI `ObjectCreate` where uiSchema was `undefined`.
* Enable OPTIONS in the browsable API.
* Updated `WritableSerializerMixin` to support URLs as pk
* Introduced new `is_url()` helper that uses Django's `URLValidator` under the hood.
* PK will be extracted from the URL before passed onto PK validation.
* Reverted a change to `readOnly` and `format` JSON schema hints that was breaking validation
* Fix a bug in getting the list display fields.
* bump for nautobot-ui
* add default headers to dcim and tenancy
* fix react warning
* fix initial column state
* add support for boolean table items

---------

Co-authored-by: Bryan Culver <bryan.culver@networktocode.com>
Co-authored-by: Gary Snider <gary.snider@networktocode.com>

* remove unused prototype code

---------

Co-authored-by: Hanlin Miao <hanlinmiao@Hanlins-MacBook-Pro-2.local>
Co-authored-by: jathanism <jathan@gmail.com>
Co-authored-by: Gary Snider <gary.snider@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Timizuo <94907097+timizuoebideri1@users.noreply.github.com>
Co-authored-by: Hanlin Miao <hanlinmiao@raleigh-dur.bear2.charlotte1.level3.net>
Co-authored-by: Hanlin Miao <hanlinmiao@Hanlins-MBP-2.home>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Introduction of new or enhanced functionality to the application
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants