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

Swagger not updating when new API endpoints added in plugin #18021

Closed
tbotnz opened this issue Nov 15, 2024 · 11 comments · Fixed by #18174
Closed

Swagger not updating when new API endpoints added in plugin #18021

tbotnz opened this issue Nov 15, 2024 · 11 comments · Fixed by #18174
Assignees
Labels
complexity: low Requires minimal effort to implement status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application

Comments

@tbotnz
Copy link

tbotnz commented Nov 15, 2024

Deployment Type

Self-hosted

Triage priority

N/A

NetBox Version

v4.1.3

Python Version

3.11

Steps to Reproduce

  1. add an endpoint to an API plugin
  2. reload netbox

Expected Behavior

swagger should update showing the new endpoint

Observed Behavior

Problem is caused by the caching of the swagger in redis, this needs to be cleared every time in order for the swagger to update. It does appear to load after the cache expires but this takes 24 hours

@tbotnz tbotnz added status: needs triage This issue is awaiting triage by a maintainer type: bug A confirmed report of unexpected behavior in the application labels Nov 15, 2024
@Bacloud
Copy link

Bacloud commented Nov 15, 2024

Same problem with v4.1.6

Copy link
Contributor

Can this be addressed with documentation, e.g. a note that if you add an API endpoint in your plugin you need to flush your redis cache (with accompanying instructions)?

@bctiemann bctiemann removed the status: needs triage This issue is awaiting triage by a maintainer label Nov 15, 2024
@bctiemann bctiemann added status: revisions needed This issue requires additional information to be actionable complexity: low Requires minimal effort to implement labels Nov 15, 2024 — with Linear
@tbotnz
Copy link
Author

tbotnz commented Nov 15, 2024

In my mind what should happen is the cached swagger should clear on container/app startup if its deemed out of date

@tbotnz
Copy link
Author

tbotnz commented Nov 15, 2024

here a simple management command that fixes it once called python manage.py $command_file_name

$command_file_name.py

from django.core.management.commands.makemigrations import Command as _Command

from django.core.cache import cache


class Command(_Command):

    def handle(self, *args, **kwargs):
        """ Temporary command to clear cache keys that are not cleared by the cache clear command. """
        _KEYS_PREFIX_REQUIRING_CLEARING = [
            "*api_schema_*",
        ]
        for key_prefix in _KEYS_PREFIX_REQUIRING_CLEARING:
            matched_keys_to_delete = cache.keys(key_prefix)
            for matched_key in matched_keys_to_delete:
                cache.delete(matched_key)

Copy link
Contributor

This is a reminder that additional information is needed in order to further triage this issue. If the requested details are not provided, the issue will soon be closed automatically.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Nov 23, 2024
@prryplatypus
Copy link
Contributor

@bctiemann what information is missing here?

@bctiemann
Copy link
Contributor

Seems like the ideal solution would be for NetBox to invalidate the cache automatically on startup, but what I'm not clear on is what the mechanism should be for it to determine that the Swagger schema is out of date. How should it "know" that anything has changed?

Alternatively, the manual cache-clearing script above would do the job, but I wonder if it would be more useful for it to just be a general "clear the entire cache" command, with an optional CLI arg to specify only the matching keys to delete (e.g. *api_schema_*), or else CLI args to target specific classes of keys, such as --api_schema. But this seems more like feature territory than a bug fix.

@prryplatypus
Copy link
Contributor

prryplatypus commented Nov 28, 2024

Seems like the ideal solution would be for NetBox to invalidate the cache automatically on startup, but what I'm not clear on is what the mechanism should be for it to determine that the Swagger schema is out of date. How should it "know" that anything has changed?

I'm not a Django power user by any means, but what if instead of caching it in Redis across all instances, it was cached in-memory, per instance, during application startup? This would eliminate the need to bust the cache at all, would keep responses to requests quick and would presumably also clear during app restarts (like when using autoreload in development or when deploying new things).

@bctiemann
Copy link
Contributor

bctiemann commented Dec 5, 2024

@prryplatypus that actually sounds like how it should work -- but looking at the code now it seems like it's already written so as to cache based on a key that depends on the release version (e.g. 4.1.7):

path(
"api/schema/",
cache_page(timeout=86400, key_prefix=f"api_schema_{settings.RELEASE.version}")(
SpectacularAPIView.as_view()
),
name="schema",
),

I think this handles the general use case of a user upgrading from one published version to another, but it doesn't address the case of a developer adding their own API endpoints within a version.

I think developers ought to be able to shoulder clearing the Redis cache manually in this scenario (redis-cli flushall). It seems reasonable to me to just document it accordingly (unless there's confirmation that the caching per-version isn't working?).

I realize (on rereading) that this is already generally known, but it feels like overkill to redesign the whole caching structure just for the sake of developers. (For that same reason I guess the management command approach suggested earlier wouldn't work either, as that would hardly be less work for the developer than just using redis-cli flushall each time they make a code change.)

@bctiemann bctiemann removed the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 5, 2024
@bctiemann bctiemann self-assigned this Dec 7, 2024
@bctiemann bctiemann added status: accepted This issue has been accepted for implementation and removed status: revisions needed This issue requires additional information to be actionable labels Dec 7, 2024
@bctiemann
Copy link
Contributor

Current proposed solution is to clear the entire Redis cache at startup if DEBUG=True. Please weigh in if it seems that is not a suitable solution.

(We don't want to flush the cache at startup in production situations because many WSGI/gunicorn workers sharing a Redis would be independently recycling on a regular basis and clearing the cache over and over; but for the purposes of development this should save the effort of clearing the cache with every code change.)

@jeremystretch
Copy link
Member

This seems very reasonable to me, so I'm going to proceed with merging the PR.

jeremystretch pushed a commit that referenced this issue Dec 12, 2024
…18174)

* Clear Swagger API cache on startup

* Clear entire Redis cache on startup if DEBUG=True
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity: low Requires minimal effort to implement status: accepted This issue has been accepted for implementation type: bug A confirmed report of unexpected behavior in the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants