Skip to content

Openapi convenience#1230

Merged
adamw merged 10 commits intosoftwaremill:masterfrom
hamnis:openapi-convenience
May 19, 2021
Merged

Openapi convenience#1230
adamw merged 10 commits intosoftwaremill:masterfrom
hamnis:openapi-convenience

Conversation

@hamnis
Copy link
Copy Markdown
Contributor

@hamnis hamnis commented May 13, 2021

Make the OpenApi types a bit more easy to work with.

  • Added a bunch of overloads to make modifications work without having to use the copy method
  • Added companion object with Empty val for ease of use.
  • Moved implementation of OpenAPI.addPathItem to Paths

Breaking:
Made Operation.operationId since this is optional in the spec, but was required in the api.
Made most case classes final, so we can rely on users not trying to extend them.
Added default values where it was possible
Schema of Parameter is optional if content is supplied

Considerations:
Parameter is probably an ADT since it has two modes of operation, with Schema or with Content.
Validation of uniqueness of Operation.operationId should be done somewhere, as uniqueness is required within one document

}

def withSummary(updated: String) = copy(summary = Some(updated))
def withDescription(updated: String) = copy(description = Some(updated))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering if the name here should be withDescription or just description? We already have some def description(s: String) methods out there, and it would be good to have a uniform approach. I'm fine with either, though we would need to come up with some naming conventions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agreed, in http4s we have started using with* prefixed methods on our domain types.
Others might prefer the unprefixed version.

Lets choose one and stick with that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I'd go with unprefixed, since we already have them e.g. in EndpointTransput.description etc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, should that include the add* methods as well, or do we keep them as is?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think add* is fine, as it is strictly different in what it's doing from the other ones

@adamw
Copy link
Copy Markdown
Member

adamw commented May 18, 2021

Thanks! Looks good :)

Replying to some of the points:

  • I think I'd keep Parameter as a single class, as it's a single concept in the OpenAPI spec. The spec says: A parameter MUST contain either a schema property, or a content property, but not both, so maybe this should be a single Either field? Alternatively this can be checked by validation.
  • If you could add convenience methods everywhere, I'm sure that would be helpful to have a uniform API
  • As for validation - that's always a hard subject. It's not only operationId, I'm sure there are other rules that need to be checked as well. Though I think it might be best to have this as an external process - sth like na OpenAPIValidator. An alternative would be to (1) throw exceptions (but we all know why this isn't perfect) or (2) replace constructors with methods on companion objects returning Eithers, but that's not perfect as well.

@hamnis
Copy link
Copy Markdown
Contributor Author

hamnis commented May 18, 2021

Re: validation
This is always a hard subject, and there is no clear answer.

Following parse not validate, we should be correct by construction, not validate after the fact.

But ergonomics matter, and having to jump into for comprehensions for constructing these types are maybe worse? I dont know.

Having a separate validator is useful maybe regardless of direction.

@adamw
Copy link
Copy Markdown
Member

adamw commented May 18, 2021

Following parse not validate, we should be correct by construction, not validate after the fact.
But ergonomics matter, and having to jump into for comprehensions for constructing these types are maybe worse? I dont know.

Me neither :) Let's maybe keep this is as a separate issue. We can always do a spike, and see how the results feel like.

Comment thread apispec/openapi-model/src/main/scala/sttp/tapir/openapi/OpenAPI.scala Outdated
@adamw adamw merged commit 167d7df into softwaremill:master May 19, 2021
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