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

fix back-reference of dynamically created route for service by pyramihook #584

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fmigneault
Copy link

@fmigneault fmigneault commented Mar 22, 2024

Using config.route_prefix and the add_cornice_service directive, I am able to register services that are used as decorators on pyramid view functions, with the appropriate prefix applied. So far, everything is ok.

However, I am also using at the same time the https://github.com/Cornices/cornice.ext.swagger package, with the help of multiple extended definitions/converters in https://github.com/crim-ca/weaver/blob/master/weaver/wps_restapi/colander_extras.py + Cornices/cornice.ext.swagger@main...fmigneault:cornice.ext.swagger:openapi-3, to auto-generate the OpenAPI 3.x definition represented by all the service paths, bodies, responses, etc. (relates to Cornices/cornice.ext.swagger#133).

When running the Swagger/OpenAPI generator, the service paths are resolved using the Service.path property. Since the services are configured with the non-prefixed path, and that prefixes are applied on the fly by add_cornice_service, I end up with incorrect paths whenever config.route_prefix is set to anything. Looking deeper into the cornice-swagger code (i.e.: https://github.com/Cornices/cornice.ext.swagger/blob/609f923784d935ba5f497e9ebb87b8e5a21747a6/src/cornice_swagger/swagger.py#L571-L593), I find that the route-builder looks for Service.pyramid_route instead of using Service.path if it is defined, in case it could extract a predefined route (aka the name definition referenced by config.add_route).

Similarly, the pyramid add_cornice_service directive looks for that Service.pyramid_route in case it already exists to use it:

def register_service_views(config, service):
"""Register the routes of the given service into the pyramid router.
:param config: the pyramid configuration object that will be populated.
:param service: the service object containing the definitions
"""
route_name = service.name
existing_route = service.pyramid_route
prefix = config.route_prefix or ""
services = config.registry.cornice_services
if existing_route:
route_name = existing_route
services["__cornice" + existing_route] = service
else:
services[prefix + service.path] = service

Now, the issue is that, when using add_cornice_service WITHOUT a predefined route, it creates one dynamically, but does not back-propagate this route reference onto the service. In order words, services[prefix + service.path] = service is set with the appropriate prefix, but the linked service in that dictionary still only has the Service.path reference without prefix.

This PR applies the route_name that is expected to be created just a few lines after onto the service.pyramid_route property.
This way, cornice-swagger is able to correctly resolve the prefixed path of the service.

# register route when not using exiting pyramid routes
if not existing_route:
config.add_route(route_name, service.path, **route_args)

fmigneault-crim pushed a commit to crim-ca/weaver that referenced this pull request Mar 22, 2024
@fmigneault
Copy link
Author

@leplatrem FYI

@leplatrem
Copy link
Contributor

Thanks @fmigneault for this detailed explanation, it looks like you went deep into the implementation.

I haven't touched this code base for a long time, and I'm wondering whether this change could have side-effects. I believe that Cornice users appreciate the very low level of activity and thus high stability. And I wouldn't want to disturb that :)
Excuse me if this sounds stupid, but wouldn't it be safer to fix cornice swagger?

If you are sure about the approach, would you mind adding a unit test to reproduce the issue you describe?

Thank you, hope that you understand my "conservatism" :)

@fmigneault
Copy link
Author

Unfortunately, the CorniceSwagger.service references are obtained from cornice.service.get_services(). It is a List[cornice.service.Service], so the prefix + service.path used as dict key in register_service_views are not making their way to the step inferring the API paths in CorniceSwagger. I think it would need important refactors to make it work from the cornice-swagger side.

Since the condition if not existing_route creates the route a bit later, I would be surprised that setting service.pyramid_route would have a side effect in this case, since it basically sets the value in the same way as if the route was provided explicitly in advance. I couldn't find any other place where service.pyramid_route is used then inside register_service_views.

@fmigneault
Copy link
Author

Well, I think I have found one uncommon side-effect with the proposed change after all.

When running a test suite with webtest for example, that employs multiple webtest.TestApp instances created for different combinations of pyramid configurations, the fact that service.pyramid_route = route_name gets defined can lead to a situation were the add_route is called only the first time the configuration went over config.add_cornice_service, but any subsequent webtest application re-including the configuration will raise a "not route named ...". This is due to a test suite using the same imported modules and cornice.Service definitions remaining in memory between test suites. Therefore, the route does not get recreated because service.pyramid_route precheck thinks it was already defined.

I have tried calling cornice.service.clear_services() before recreating the config, but that doesn't seem to be enough. The services remain in memory somewhere.

Do you have any idea how this kind of problem could be resolved?

fmigneault added a commit to crim-ca/weaver that referenced this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants