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

Hide endpoints from OpenAPI schema by default #1196

Merged
merged 15 commits into from
Aug 1, 2023

Conversation

DaelonSuzuka
Copy link
Contributor

@DaelonSuzuka DaelonSuzuka commented Jul 19, 2023

Implementation of #1188

There's not much to say, this is pretty simple. The tests were definitely more work than the feature, but the existing tests had everything I needed to steal borrow.

Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for the great addition.

@falkoschindler falkoschindler self-requested a review July 19, 2023 10:59
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks, @DaelonSuzuka, for this great PR! I'd love to improve NiceGUI's OpenAPI support.

There are, however, several concerns with the current implementation. Some are mainly aesthetics, others are more serious:

  1. For consistency with the uvicorn_reload parameters, I'd like to support comma-separated lists that are processed with the very same split_args functions.

    But since we're not dealing with arbitrary strings, a List[Literal['internal', 'page', 'all']] might be even better. This would support auto-completion and static type checkers without adding too much user code (['internal', 'page'] instead of 'internal page').

    Actually we only need one string at the moment: "", "internal", "page", or "all". So I could imagine declaring the type as Literal['none', 'internal', 'page', 'all'] = 'none' for now. If needed, we could add additional support for lists of literals later.

  2. The check 'internal' not in endpoint_documentation is not very precise, as it would match for something like "no_internals". Of course, this is related to points 1. Using a stricter type for the parameter has impact on the way we can implement conditions. If we go with a single literal, we can write if endpoint_documentation in {'internal', 'all'}: ....

  3. I think we also need to consider ui.run_with(). Both functions ui.run() and ui.run_with() could share the code for setting the include_in_schema attributes.

  4. I experimented a bit with the pytests, shortened the code a little and remove the page decorators, because the auto-index page should be enough for these test cases. But some tests fail and I don't know why. Looks like we need tests with and without page decorators.

  5. Routes might be created later. Especially the app.add_static_* methods add routes automatically. But there might be even more places we need to consider.

@DaelonSuzuka
Copy link
Contributor Author

DaelonSuzuka commented Jul 19, 2023

Great feedback, I agree with all of it except maybe the run_with() stuff. I have thoughts about run_with() that I'll return to later because they're much bigger in scope.

types

Fwiw, I like Literal['none', 'internal', 'page', 'all'] = 'none' the best so far. I really can't imagine what other categories would need to be added.

Routes might be created later.

Funny story, I actually implemented the whole thing that way first, realized that normal routes get processed before run(), and rewrote it like this. I can easily re-add that original version.

tests

Wow, it's embarrassing how much cleaner you made those. I see this is going to be another project with a high educational value from contributing.

@DaelonSuzuka
Copy link
Contributor Author

DaelonSuzuka commented Jul 24, 2023

Routes should now be correctly filtered whether they're created before or after ui.run(). It works running locally on a little test application, but two of the tests still aren't passing, and it took me a good while to figure out why: the test harness is creating a custom route for "/" that doesn't match the usual patterns. (see: https://github.com/zauberzeug/nicegui/blob/main/tests/conftest.py#L44)

I could add that case to the filter, but I think that creates a chance for colliding with legitimate names from user-created API-endpoints? Advice/opinions welcome.

Especially the app.add_static_* methods add routes automatically. But there might be even more places we need to consider.

app.add_static_files() (https://nicegui.io/documentation#static_files) and app.add_media_files() (https://nicegui.io/documentation#media_files) aren't as immediately obvious as ui.page() as far as desired behavior goes. Maybe they should follow the internal flag ?

Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

I just reviewed the code once again. Looks good to me. 🙂

But of course, we need to fix the failing tests. @DaelonSuzuka Can you elaborate on the choice of these two conditions:

if route.path.startswith('/_nicegui') and hasattr(route, 'methods'):
    # handle internal route
if route.name == 'decorated':
    # handle page route

I'm not sure if they are stable enough and catch all cases. Especially the auto-index page "/" doesn't seem to be correctly handled, probably because it isn't decorated. I couldn't find a quick fix. But maybe you help me understand the current approach before I try to improve it.

@DaelonSuzuka
Copy link
Contributor Author

DaelonSuzuka commented Jul 26, 2023

@falkoschindler There wasn't much of an approach, tbh. I made an example app with some ui.page()s and used the debugger to inspect globals.app.routes and look for patterns in the route objects that I could use for filtering.

FastAPI.get() has an optional keyword arg name that we can use to set the route's name specifically. That's usable every place I can find a route getting defined (including in tests/conftest.py, which will fix the failing tests). We can set the names to something like _nicegui_internal and _nicegui_page and then the filtering logic will be cleaner. This should also be enough to prevent accidentally removing routes from other sources.

I don't know how common it is for FastAPI users to set names on routes. I've never seen it in a tutorial or stack overflow answer, but that's hardly definitive. A user wanting to set custom names as a kwarg to ui.page() would then conflict with us setting that param, and either we'd ignore their name or break the doc hiding behavior. I don't think either of these outcomes are terrible problems, so maybe just improve that behavior if somebody complains later?

@falkoschindler
Copy link
Contributor

Ok, I think I could fix both tests by checking if the route is in globals.page_routes.values().

But the tests still fail when run all in one go. It looks like something isn't reset between individual tests.

@falkoschindler
Copy link
Contributor

Yay, I managed to reset the openapi_schema between tests. Now they all pass.
What do you think, @DaelonSuzuka and @rodja? Ready to merge?

@falkoschindler falkoschindler added this to the 1.3.7 milestone Jul 27, 2023
@falkoschindler falkoschindler added the enhancement New feature or request label Jul 27, 2023
@falkoschindler
Copy link
Contributor

Well, in combination with all other tests, the endpoint tests fail. 😕

But maybe it is related to the current problem with the Chrome driver. I had to create driver manager like this to get the tests working at all:

s = Service(ChromeDriverManager(version='114.0.5735.90').install())

Even with this hack some tests fail for the v1.3.6 tag, let alone this branch. So I think we should fix the test infrastructure before merging new features.

@DaelonSuzuka
Copy link
Contributor Author

I read the commits and your changes are much cleaner, but I pulled the branch to my local machine. I have the same issue with the unit tests, but I also found this problem:

image

User-provided routes other than / don't appear in the api docs at all now. I can figure out why, but I won't have time to work on this until the weekend.

So I think we should fix the test infrastructure before merging new features.

No objections here.

Possibly related side note: I did have trouble getting NiceGUI installed in the first place. It felt like the requirements were out of date or were only tested on one OS. I also couldn't get the development docker environment to work, but I didn't put much effort into diagnosing why.

@falkoschindler
Copy link
Contributor

@DaelonSuzuka Weird. I thought our tests would cover this case. Well, we'll need to look into it. I won't have time over the weekend, but maybe you'll have some insight.

Our docker setup is probably not in the best shape right now. There are multiple related issues and PRs working on it: #1255, #1057, #1018.

@DaelonSuzuka
Copy link
Contributor Author

image

Well that was easy, basically just a typo. My manual testing all works now.

Off topic:

I do find it a bit strange that globals.page_routes is a dict[<function>:<path>] instead of the more intuitive dict[<path>:<function>], but there's probably a reason I'm not seeing.

The test suite fails with stuff like "ValueError: There is no such driver by url https://chromedriver.storage.googleapis.com/LATEST_RELEASE_115.0.5790", which I believe you're already aware of. I'd love to help but I have absolutely no experience with selenium, and honestly not enough experience with an unusual test rig like this, so I think I just have to wish you good luck on that front.

@falkoschindler falkoschindler merged commit 649d6f8 into zauberzeug:main Aug 1, 2023
1 check passed
@falkoschindler
Copy link
Contributor

Ok, we finally fixed the pytests and merged into main. Thanks again, @DaelonSuzuka!

Regarding globals.page_routes: The initial purpose of this dict was to find the corresponding route path for a page function. Therefore it maps from function to string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants