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

Transform api operations based upon middleware wrappers #269

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

Conversation

platy
Copy link
Collaborator

@platy platy commented Nov 27, 2020

I came across the need to modify the api spec based on properties of middleware transformers. In our case it was RBAC middleware which specified different roles for different endpoints.

This change adds another trait to the parameter on the Resource wrapper, TransformApi. In this use case it allowed the roles required on the endpoints to be automatically added to operation.tags and into operation.description, which seems like it will be very useful.

I have a couple of concerns about it though:

  • this changes the signature of wrap so it is a breaking change for users of that function - it could be given a new name
  • I've made RouteWrapper public - maybe it should use a different type?
  • the transformers need to be cloned to be passed into the inner and kept in the wrapper - but in my case they are very small
  • the transformers are being kept in the wrapper in a type-system linked list, this avoided allocations and is fine in my case but maybe others would have a problem with it

@platy platy requested a review from wafflespeanut November 27, 2020 09:43
@dunnock
Copy link
Collaborator

dunnock commented Nov 27, 2020

BTW, would not the roles required on endpoint better managed with Apiv2Security derive?
We are managing this internally via FromRequest handlers on a User and AccessToken, that seems like better fit in actix world. Not sure about your use case details though.

In regards to back compatibility - we are doing 2 phase initialization of actix-web, first is for all services which we exclude from schema, then doing wrap_api and all schema based initializations. I guess this can be adviced as a migration approach if your PR would be accepted.

@platy
Copy link
Collaborator Author

platy commented Nov 27, 2020

Yes I think it might be a nice solution to have a security parameter which contained type information about the roles that are allowed access and to implement the schema documentation there. But for now that seems to big of a change for this project. The role information is already there in the middleware and available at runtime, it seems reasonable that middleware could alter the spec as it could change anything about the handling of resources.

I have only worked on this one actix web project so I don't know anything about how they generally look, but I feel that more extensibility would be helpful to paperclip.

@platy platy force-pushed the wrap-api-transform branch from 5f3964d to 9d91db9 Compare November 27, 2020 11:08
@dunnock
Copy link
Collaborator

dunnock commented Feb 15, 2021

There is a different way to pass data from middleware now ReqData so that might be a better way to modify the schema via wrapped types? #300

@platy
Copy link
Collaborator Author

platy commented Feb 22, 2021

@dunnock Thank's for the info - I'll keep that in mind if we update to actix-web 3 at some point

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

Successfully merging this pull request may close these issues.

2 participants