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

Codegen: Generate attributes on endpoints for any specification extensions defined on path or operation objects #3599

Merged

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Mar 13, 2024

The OpenApi spec permits defining Specification Extensions on a large number of components. Not all of those have any kind of obvious mapping into the tapir model; however it feels natural to map extensions on path and operation definitions into .attribute declarations on generated endpoints, thus making this metadata accessible at runtime for $variousSundryPurposes

For context, this would enable me to implement some specific authorization guards cleanly at the implementation layer without having to hack around with tags. I don't think tapir would currently emit any specification extensions when generating the openapi from an endpoint with attributes, so I guess there's a break in idempotency there? But since we currently ignore extensions anyway I don't consider that a new issue and think this should be ok... Happy to engage in alternatives to solve the same problem! But I do think this is the most sensible route

@hughsimpson hughsimpson force-pushed the map_x-directives_to_attributes branch 3 times, most recently from fc32b5f to cde47d7 Compare March 15, 2024 10:22
@kciesielski
Copy link
Member

kciesielski commented Mar 18, 2024

Thank you. Technically this should work, but I'm a bit concerned about using AttributeKey[String] for each property. AttributeKey is meant to represent an unique type, and the internal typeName field normally represents its fully qualified name. For extensions, we could have unique value classes like case class XSomeExtensionProperty(value: String) extends AnyVal (or Long, or Map etc.) instead of an AttributeKey[String]("x-some-extension-property"). Wdyt @adamw?

@adamw
Copy link
Member

adamw commented Mar 18, 2024

Maybe we could generate a single typed attribute SpecificationExtensions, which would also allow looking up the values in a type-safe way?

@hughsimpson
Copy link
Contributor Author

hughsimpson commented Mar 18, 2024

Thanks for the comments. The type of the key should've been the right type for the value as specified (rather than always being String), but it did permit multiple types for the same attribute which I guess is wrong. I'll have a fiddle around with this.

@hughsimpson hughsimpson force-pushed the map_x-directives_to_attributes branch from 95709e1 to bee2ad3 Compare March 18, 2024 13:22
@hughsimpson
Copy link
Contributor Author

I've pushed up a change that I think would be more in line with how attributes are designed (although see other comment - I need to improve the type logic in the conflict check). Let me know if this looks like the right path

@hughsimpson hughsimpson marked this pull request as draft March 19, 2024 17:21
@hughsimpson
Copy link
Contributor Author

Marked as draft for now, shoulda done that earlier, sorry

@hughsimpson hughsimpson force-pushed the map_x-directives_to_attributes branch from bee2ad3 to faf1012 Compare March 20, 2024 15:45
@hughsimpson hughsimpson marked this pull request as ready for review March 20, 2024 15:47
@hughsimpson
Copy link
Contributor Author

hughsimpson commented Mar 20, 2024

I've updated the logic around the keys; we should now construct and use a single attribute key with the correct LUB parent type for each specification extension key. Let me know what you guys think.

@hughsimpson
Copy link
Contributor Author

I could add a parameterised case class here for wrapping the type, maybe? Something like XSpecificationExtension[T]? Kinda easy on the specifics really, just want a way to pass it through.

@kciesielski
Copy link
Member

I could add a parameterised case class here for wrapping the type, maybe? Something like XSpecificationExtension[T]? Kinda easy on the specifics really, just want a way to pass it through.

Sounds good to me :)

@hughsimpson
Copy link
Contributor Author

OK let's try this. Let me know.

@kciesielski
Copy link
Member

Nice idea with type aliases. Isn't it enough to have just one level then, without the generic wrapper? AttributeKey[CustomListExtensionOnOperationX] has a different .typeName than AttributeKey[AnotherListExtensionOnOperationX], which is what we were after.

@hughsimpson
Copy link
Contributor Author

Yeah I prefer it with the single layer too tbh, just thought it was worth trying it out to see how it feels. I'll revert the last pr.

This reverts commit e5e0f1c.
Copy link
Member

@kciesielski kciesielski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @hughsimpson!

@kciesielski kciesielski merged commit a2eca04 into softwaremill:master Mar 27, 2024
23 checks passed
@hughsimpson hughsimpson deleted the map_x-directives_to_attributes branch March 27, 2024 11:40
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.

3 participants