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
Introduce new OpenApi component #49573
Conversation
27d0476
to
45e78a2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: T1l3 <tib.pierre@gmail.com> Co-authored-by: cmeniger <cmeniger@gmail.com> Co-authored-by: nicolas-nielsen <nnielsen.dev@gmail.com> Co-authored-by: senzowayne <actu-senzo@hotmail.fr>
This comment was marked as resolved.
This comment was marked as resolved.
45e78a2
to
d115cd7
Compare
What's the difference with:
It's not the case for these two examples.
I don't see why you couldn't create multiple OpenAPI documentations with these two examples. |
@alanpoulain Thanks for your question! I didn't know about https://github.com/api-platform/core/tree/main/src/OpenApi, it seems a very similar feature indeed! I don't know why it's not a dedicated package, do you know? I knew about https://github.com/cebe/php-openapi and I think it's very different: it's a parser/writer for JSON/YAML packages, it doesn't provide the DX capabilities to define a documentation, AFAICT. |
I ask myself: how do you plan to handle the different OpenAPI versions ? For exemple nullability is handled in a very different way if you are using |
@Korbeil yes, it is indeed a big challenge. The first initial idea I have is to act differently depending on what features changes: if different versions are compatible, we can implement them in minor versions. If we have incompatible standard changes, we could want to synchronize the standard upgrade to the next major version, but I'm not 100% clear of whether this would work. |
*/ | ||
interface OpenApiBuilderInterface | ||
{ | ||
public function schema(SchemaConfigurator|ReferenceConfigurator|string $definition = null): SchemaConfigurator|ReferenceConfigurator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As SchemaConfigurator and ReferenceConfigurator don't have the same API and the are returned for different kind of input, I'm wondering whether it is a good idea to have such a union type as return type. SA tools will force you to restrict the type when the method you want to use is not one of the common ones, and IDE completion will generally suggest methods from both classes while this won't always be valid either.
* @author Titouan Galopin <galopintitouan@gmail.com> | ||
* @author Selency Team <tech@selency.fr> | ||
*/ | ||
interface ComponentsLoaderInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Qualifier nouns are singular in English. Why is Components
a plural here ?
documentations using PHP objects, giving more flexibility and reusability to OpenApi | ||
definitions. All features of OpenApi 3.1 are supported. | ||
|
||
### Writing documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be in the readme itself but in the symfony-docs repo (that's what the symfony-docs PR item is for in the PR template, while you linked to an issue).
If you want to make this doc available in the PR during initial review of the component to make it easier, I suggest you to move it to a separate file (that will be removed from the PR before merging it), using the RST format (so that it will be ready to be moved to the docs repo).
*/ | ||
interface DumperInterface | ||
{ | ||
public function dump(OpenApi $compiledDoc): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this DumperInterface is useful. I doubt we will have code that can work with any dumper without knowing ore about them (and we might have cases where we would want to dump a compiled doc to a static website for instance, which would not fit with returning a string)
|
||
public function configure(DocumentationConfigurator $doc): void; | ||
|
||
public function loadComponents(DocumentationConfigurator $doc, ComponentsLoaderInterface $loader): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for those interfaces is missing. We need phpdoc to explain the purpose of those methods.
In particular, loadComponents
seems confusing to me as it looks like it can be called multiple times during the compilation of a documentation.
* @author Titouan Galopin <galopintitouan@gmail.com> | ||
* @author Selency Team <tech@selency.fr> | ||
*/ | ||
class CallbackRequestConfigurator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should all those configurator classes be final ?
* @author Titouan Galopin <galopintitouan@gmail.com> | ||
* @author Selency Team <tech@selency.fr> | ||
*/ | ||
trait ContentTrait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making all those traits @internal
public function example(mixed $name, ExampleConfigurator|ReferenceConfigurator $example = null): static | ||
{ | ||
if ($example) { | ||
$this->examples[$name] = $example->build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this validate that the name is a string in that case ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and maybe those 2 cases should be different methods
{ | ||
private ?ExternalDocumentation $externalDocs = null; | ||
|
||
public function externalDocs(string $url, string $description = null, array $specificationExtensions = []): static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding the phpdoc to describe the type of the array (based on the ExtensionsTrait, I suppose it is array<string, mixed>
)
|
||
foreach ($this->selfDescribingSchemas as $selfDescribingSchema) { | ||
$configurator = $openApi->schema()->type('object'); | ||
\call_user_func([$selfDescribingSchema, 'describeSchema'], $configurator, $openApi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use $selfDescribingSchema->describeSchema($configurator, $openApi)
instead. There is no need for call_user_func
We plan to do a subtree split in API Platform soon (that's why the namespace has changed for API Platform 3).
You can totally use |
I agree with @lyrixx about not being able to tie the OpenApi version to the Symfony version. This would mean that updating Symfony would force you to update the version of OpenApi you generate, while projects might have a need to keep generating specs using the older OpenApi version (because other tools they have is not compatible yet with the new version for instance) |
@tgalopin thanks for this work! I think we could improve your initial example, to show how we could leverage all other Symfony components to configure the documentation of an API. Here's a basic example with the router: use Symfony\Component\OpenApi\Documentation\AbstractDocumentation;
class Documentation extends AbstractDocumentation
{
public function __construct(
private readonly RouterInterface $router
) {}
public function getIdentifier(): string
{
return 'myapi';
}
public function getVersion(): string
{
return '1.3.4';
}
public function configure(DocumentationConfigurator $doc): void
{
foreach ($this->router->getRouteCollection()->all() as $route) {
$doc->path($route->getPath(), $this->openApi->pathItem();
// ..
}
}
} And we can imagine bunch of usage, with the container, entities, DTOs, PropertyAccess, PropertyInfo, etc @alanpoulain about the OpenApi part of ApiPlatform you mentioned, I have serious issues with the generated openapi content. I never managed to understand what the problem was, as it's very coupled with the rest of the framework. About cebe/php-openapi, I didn't know it. |
Because we needed to finish refactoring API Platform 2.x before being able to think about a subtree split. By the way, the About your implementation, the "configurator" pattern looks quite overkill, complex types that all union to About handling multiple OpenAPI versions I really don't think benefits to anyone, the OpenAPI initiative should be pushed forward and always use last versions. If a user wants modifications, at least in API Platform, he has the ability to change everything inside a Normalizer, which gives full autonomy. We do it in the framework to handle a compatibility with AWS Gateway. Last but not least, the hard part about OpenAPI is about JSON Schema, and most breaking changes within that specification will probably come from the JSON Schema specification itself, not the OpenAPI one. I'm confused about this proposal, especially that the backward compatibility policy of Symfony is not compatible with how OpenAPI/JSON Schema evolves.
@welcoMattic it can be un-coupled, create your factory based on the route collection like you just did in your message. I'd be interested to know what use case is not possible to do with the OpenApi component of API Platform. |
Even if I really like the idea, the tooling required for
And not really, it depends on which OpenAPI version you're talking about. |
I'm not against having an OpenAPI component in Symfony, and it seems to be a nice work (but I agree with @soyuka's opinion on the matter, the DX/API seems hard to understand), but it would have been great to have a conversation with API Platform core team beforehand. |
@lyrixx do you mean we would create a namespace for each version of OA in the component? I'd be totally fine with that indeed @alanpoulain I wouldn't say that https://github.com/cebe/php-openapi#writing-api-description-files helps a lot at writing OpenApi specifications, it's very manual still (no autocompletion, no helpers, ...), but I get your point. I'd be totaly fine simplifying the code of this PR to better integrate ideas from the API Core repo :) ! BTW, I did talk about it with Kevin and @soyuka before releasing, and it's just a PR, it's up for debate ofc :) ! |
OK I didn't know. An issue with this component IMO: it should probably be split into two components: JSON Schema and OpenAPI. |
I tried to do cloning first (just like API core does AFAICT) but I encountered an issue: cloning means having the OpenApiBuilder inside the entities themselves, which made the code less readable IMO. Having the OpenApiBuilder available is important because it allows to override it on user-land side: class OpenApiBuilder extends BaseBuilder
{
public function paginatedCollectionSchema(SchemaConfigurator|ReferenceConfigurator|string $itemSchema): SchemaConfigurator
{
return $this->schema()
->type('object')
->property('items', $this->schema()->type('array')->items($itemSchema))
->property('pagination', Documentation::REF_SCHEMA_PAGINATION_DETAILS)
;
}
} |
Very interesting, I'm not a fan of the platform api because it is extremely rigid and not flexible at all. Having a component like the one you suggested where you can freely define your documentation is a really great contribution to the community. Even if the component is not accepted, I would suggest that you release it as a personal project. Good job @tgalopin. |
I'd be fine with that indeed, up for debate I don't have a strong opinion on the matter. Creating a package does mean more time to maintain it (packagist, subtree split, ...) so I guess it needs to have a good reaons to be? |
@soyuka also: using the serializer is fine to me, it's just that the models do sometimes need code manipulation and it seemed unecessary to require the serializer for simple JSON encodings |
For instance a nice reason could be to have a JSON Schema validation constraint in Symfony. |
Why it would be better than defining this spec in raw yaml or json file? It is universal format. |
@alanpoulain indeed, it could be useful, let's gather more opinions on this |
@jakublech we actually started to do it manually, but it's very cumbersome. We repeated ourselves a lot, we weren't able to reuse parts of our definitions around, we couldn't store the documentation close to the code linked to it,... Also, the component provides reusability capabilities, for instance : class MyOpenApiBuilder extends BaseBuilder
{
public function paginatedCollectionSchema(SchemaConfigurator|ReferenceConfigurator|string $itemSchema): SchemaConfigurator
{
return $this->schema()
->type('object')
->property('items', $this->schema()->type('array')->items($itemSchema))
->property('pagination', Documentation::REF_SCHEMA_PAGINATION_DETAILS)
;
}
} |
Please don't do that.
Adding more methods in a child class of a Symfony class is not covered by the BC promise (as that would require forbidding to add new methods in existing Symfony classes that are not final). And your proposal would not help much as the IDE would not provide completion for your new method AFAICT (or if it does thanks to a local property holding an object, you could put such method in a separate dedicated class). Regarding serialization, I think the benefit of an external serializer is that you could implement different serialization for OpenAPI 3.0 vs OpenAPI 3.1 while keeping the same model. |
To me, a JSON Schema validation constraint is not a use case for the component described here. Such a constraint will need a parser for a schema, not a writer for it (unless you want to recreate all schemas you receive in the standard json format) |
Why not just contribute the missing |
@stof If we want a component (or two) in Symfony, a parser is a must have. |
We started to talk about it in the hall of the Symfony Con but we couldn't really get deep into it, I'd have liked us to discuss this matter longer. I think that API Platform is now mature enough and I think that @bebetoalves sentence is quite wrong about its rigidity. Also, I do think that @tgalopin would be able to do exactly what he wants using some of API Platform components (OpenAPI being one of them). Quite not the subject. About the "configurator" pattern I don't find it appropriate and think it is a huge overhead for no benefits. Having immutable classes with withers and a constructor you use with named arguments is more user-friendly (completion) and reduces complexity. In our design we have two extension points:
I think that @stof message from above is really in sync with what I already said. Obviously the suggestion of the route collection is figurative and performance is one of the reason we're using the API Platform metadata system and not the router. I also understand that there is quite some frustration using API Platform's OpenApi component. I'd like to gather these frustrations as I do think they're quite easily solvable. You can reach me on the Symfony slack or open an issue on API Platform core. Also, we're going to work on our subtree split so that everyone can experiment and re-use some of our top notch components without needing to use the whole framework. As this subject receives quite some attention, I suggest that we try not to pollute this issue too much and maintain focus on the technical aspect of this component. |
@dunglas @soyuka perhaps instead of a subtree split in api-platform, the code could be put in Symfony as the OpenApi component (considering it'd need to be decoupled from apip to do so)? I don't think apip is (currently) known for its reusable components, meaning the package would get more visibility in the Symfony organization. I'd be happy to update my PR here if that seems like a good idea. |
I'm not sure its in Symfony's (maintainers) best interest as we said a few times already the compatibility policy, and the release schedule may not fit well with how OpenAPI specification moves. OpenAPI Models under the The subtree is on our list and is actually top priority for us since the 3.1 release, not only for OpenAPI. Let's see if the Symfony core team wants an OpenAPI component. |
Also, as their names imply, both OpenAPI and API Platform are all about (web) APIs. It has always been the goal to provide reusable standalone components (even in v1). We ready provide standalone components for our JS/TS code, and I would be very happy to also finally manage to provide standalone PHP libs. We always contributed the generic (non-API-related) components and features to Symfony (DI autowiring, most Serializer features, WebLink, many Security features, Docker support for Flex...) and we'll continue. But I think that it's better for both API Platform and the Symfony project to keep the API-related code in API Platform, and keep Symfony focused on the lower level and "HTML websites" use-cases. WDYT? |
As big fan of the design-first approach to build APIs, structuring it the way API-Platform does through small value objects is a definitive superior DX experience compared to the Configuration approach suggested here. On the idea of Symfony supporting this package, I agree that it could conflicts with the organisation update policies. As a hypothetical consumer of this package, I would like to keep the freedom of bumping the package version every time there's a new spec to support. Not waiting for a whole Symfony release cycle.
Just a note on what @dunglas said. I disagree, let's not make API Platform the de facto solution for symfony users that would like to do API stuff in their symfony app. It's still perfectly doable to produce a fully-fledged API on top of Symfony alone. I think Symfony should continue to support any type of architecture, the same way it did by moving to a micro kernel, removing hard dependencies to the template system. Things that would be qualified as mandatory for "HTML websites" for instance. |
That's definitely something that is not possible if this package goes in the Symfony monorepo.
Making it a standalone package would not force you to use ApiPlatform to use that package. |
Wow, a lot of work and a good result. But I don't really see the big benefit for the symfony community because IMHO this wouldn't fit with the processes. In my eyes creating the OpenAPI definition from the code and possibly the entities is the wrong approach. And having a method I would be rather against this component to include but this as a library e.g. to share in the private account. BTW: Pershaps we should also think about AsyncAPI in this context. |
Today the ecosystem is already fragmented:
I disagree with what @stof said:
API Platform is currently using a lot of Symfony packages, and like @dunglas said we try to backport everything back in Symfony. If we want new components in Symfony, we should strive for a common approach to make sure everyone can use them.
Without these criteria covered, I don't think adding this component to Symfony would be the best idea. |
@alanpoulain I'm not saying we need different components in Symfony and API Platform. I'm saying that API Platform might provide a standalone library that they maintain, that can also be used independently of API Platform, with no other library provided in Symfony. |
FYI Janephp use the json schema (i.e. https://github.com/OAI/OpenAPI-Specification/blob/main/schemas/v3.0/schema.json) to generate models and normalizer (that are based on symfony serializer component) This allow us for each new schema version / update to regenerate those files and have static analyze tool detecting errors / problems with the new version (like a deleted field in a new version that we use) |
@stof OK I didn't fully understand, sorry.
Isn't it the case for some other components? It's not an easy task sure but it's not impossible. OpenAPI has evolved quite a lot between 3.0 and 3.1 but I don't think it will happen again soon. The ecosystem (in JS notably) has a hard time to keep up. |
@symfony/mergers before spending time on the component, I'd love to have your input to see if we think it's a good idea to have such a component in Symfony. I'd be totaly fine publishing it under a Selency name (I do think it's a useful package, especially with the new payloads introduced by #49138) but it doesn't have to be under Symfony namespace; |
I think it's fine to keep such API specifics under the API Platform org given how well established is the collaboration, and they already have all the foundation for this. |
To me, this component aims to help developer to document an API built with Symfony. Not necessarily with API Platform. So I would like to see the OpenAPI (and JsonSchema) components under Symfony org, as it is still possible to create API without API Platform. However, we must build these component with API Platform team to be sure they could be reused easily in API Platform applications. |
The thing is that just like Symfony, API Platform is not only a framework but a set of components meant to cover API-related needs. And this will become even more true in the upcoming versions as they're working on splitting and shipping every single component solely (and I guess this initiative is gonna accelerate the process). A similar situation could happen with e-commerce stuff, which I'd rather suggest to put in the Sylius org beside their other reusable components. |
TL;DR:
Recently, I had to work on API without using FOSRest / API Platform / Nelmio API Doc. The need was to add documentation on internal API and public API (for third parties). API Platform team can correct me but If I understand correctly, the approach of the standalone package will support only one specification version at a time, the latest. Bumping from 3.0 to 3.1 is a breaking change for anybody relying on the documentation because of the BC included in the new version of the specification. I dunno how dependencies will be defined in API Platform but if bumping API Platform version leads to having a new version of the specification, it would be a maintenance problem quick fixed by sticking to patch upgrade and I'll be coupled to API client to have greenlight on framework upgrade. 3.2/4.0 specification are far away but 3.1 was released two years ago and most tools still does not support it yet (or partially). Regardless of which orgs will propose a component, I'll prefer an approach where I can specify which version I want to build. I'll be sure to always build the documentation at the expected version and even more, be able to add version to the documentation itself (i.e. /v3.1/openapi.json, /v3.0/openapi.json), communicate about deprecation and still be able to upgrade when I plan to. |
The openapi plugin in PHPStorm, or even stoplight.io already handles this part quite well. Instead you will get autocomplete from a proxy to generate the openapi doc... which might deverge at some point. I don't really see the point to be honest. |
Stoplight.io is a mess to use and a separate IDE. I lost so many times trying to use it (and losing info) prior going to manually write it. Both Stoplight and PHPStorm don't have support of 3.1 (or at least not all features). Yet, both let pass 3.0 issues that Redoc validation didn't. PHPStorm is complaining about unresolved |
Still... An IDE can provide a good autocomplete and we shouldn't try to fix this. The autocomplete doesn't seems to be argument enough to me. |
PHPStorm was able to generate Controller, Command for years. DX is not only about autocompletion. |
I'm closing this PR given the code has been released under the Selency organization (https://github.com/selency/openapi) and API Platform extracted their OpenAPI component meanwhile (https://github.com/api-platform/openapi). |
This PR proposes the introduction of a new OpenApi component to the Symfony ecosystem. The component aims at providing a user-friendly, object-oriented API to build OpenAPI specifications using PHP.
This component's proposal comes from difficulties we (at Selency) encountered while trying to implement a high-quality documentation using alternatives (like NelmioApiDocBundle or swagger-php):
To address these issues, our proposal is instead to rely on PHP and its capabilities: in the same way we can now define services in PHP with autocompleted method names and linting, this component proposes the ability to describe an OpenApi documentation in PHP, and then dump it as JSON or YAML for usage in other tools. All features of OpenApi v3.1 are supported.
Simple usage example
You can find more advanced usages in the README.
If this component is approved, I plan to do the FrameworkBundle integration right after to automate several aspects of its usage: self-describing schemas/query params loading, openapi directory creation in the project, ...