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

Allow finding child attributes in method args #1368

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

ruscon
Copy link
Contributor

@ruscon ruscon commented Dec 20, 2022

This allows you to create custom attributes like this:

use OpenApi\Attributes\Parameter;

#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::TARGET_PROPERTY | \Attribute::TARGET_PARAMETER | \Attribute::IS_REPEATABLE)]
class QueryParameter extends Parameter
{
    public $in = 'query';
}

And use it in a class method like this:

public function index(#[QueryParameter] ?string $name) {}

A similar solution can be done for Cookie, Header and RequestBody, I can try to implement them inside the library itself, if you want

Copy link
Collaborator

@DerManoMann DerManoMann left a comment

Choose a reason for hiding this comment

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

For completeness it would be good to also add a QueryParameter annotation class.

Also, if you do a search for PathParameter in the src folder you'll find a lot more places where QueryParameter would need to be added.

Finally, it would be cool to go through the Examples folder and the test fixtures and change a few Parameter annotations to QueryParameter and make sure the tests still pass.

@@ -53,7 +53,7 @@ public function build(\Reflector $reflector, Context $context): array
// also look at parameter attributes
foreach ($reflector->getParameters() as $rp) {
foreach ([OAT\Property::class, OAT\Parameter::class, OAT\PathParameter::class] as $attributeName) {
foreach ($rp->getAttributes($attributeName) as $attribute) {
foreach ($rp->getAttributes($attributeName, \ReflectionAttribute::IS_INSTANCEOF) as $attribute) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good find! I didn't know about the flag yet.

In fact, with the flag the PathParameter::class element can be removed from the foreach I suppose?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been thinking about this before - hence the PathParameter annotation.

I would argue that path and query are the most common cases and therefore we limit it to those two.

Copy link
Contributor Author

@ruscon ruscon Dec 20, 2022

Choose a reason for hiding this comment

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

In fact, with the flag the PathParameter::class element can be removed from the foreach I suppose?

Yes, it's true :)

I've been thinking about this before - hence the PathParameter annotation.

Yes, it can be removed :)

Copy link
Contributor Author

@ruscon ruscon Dec 20, 2022

Choose a reason for hiding this comment

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

@DerManoMann one additional thing, for Parameter you have the ParameterTrait, but for RequestBody you don't.

In fact, for each base class, you need to create a trait, then it will be very easy to inherit any annotation/attribute class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I can follow you here...

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruscon Let me know if the above works for you...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DerManoMann Do you mean, removing PathParameter?
Yes, it will work, but if somebody already uses it, you will create a breaking change.

Copy link
Collaborator

@DerManoMann DerManoMann Dec 22, 2022

Choose a reason for hiding this comment

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

No, I mean

                    foreach ([OA\Property::class, OA\Parameter::class] as $attributeName) {
                        foreach ($rp->getAttributes($attributeName, \ReflectionAttribute::IS_INSTANCEOF) as $attribute) {

It removed the dependency on PathParameter in that code path and allows for other custom attributes that extend from the Property and Parameter annotation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that the alias changed from OAT to OA, so any attributes extended from the annotation base classes will match.

Copy link
Contributor Author

@ruscon ruscon Dec 23, 2022

Choose a reason for hiding this comment

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

Note that the alias changed from OAT to OA, so any attributes extended from the annotation base classes will match.

@DerManoMann Yep, it works with OA.

Some tests failed.

1) OpenApi\Tests\Annotations\AttributesSyncTest::testPropertyCompleteness with data set "HeaderParameter" ('OpenApi\Attributes\HeaderParameter')
ReflectionException: Class "OpenApi\Annotations\HeaderParameter" does not exist

should I add an Annotation version for the new Attributes?

P.S. probably it will be good to refactor the Annotation and Attribute classes.
It would be nice if they were in the same class.
Example

Right now we will have a stupid inheritance:

OA\AbstractAnnotation
└── OA\Parameter
    ├── OAT\Parameter
    │   ├─ OAT\QueryParameter
    │   ├─ OAT\PathParameter
    │   ├─ OAT\CookieParameter
    │   └─ OAT\HeaderParameter
    ├── OA\PathParameter
    ├── OA\PathParameter
    ├── OA\CookieParameter
    └── OA\HeaderParameter

because of this:

namespace OpenApi\Attributes;

#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::TARGET_PROPERTY | \Attribute::TARGET_PARAMETER | \Attribute::IS_REPEATABLE)]
class Parameter extends \OpenApi\Annotations\Parameter
{
    use ParameterTrait;
}

@ruscon ruscon force-pushed the patch-1 branch 2 times, most recently from 2eff4ff to 1fcf960 Compare December 23, 2022 01:25
@@ -40,7 +40,7 @@ public function __construct(
) {
parent::__construct([
'parameter' => $parameter ?? Generator::UNDEFINED,
'name' => $name ?? Generator::UNDEFINED,
'name' => Generator::isDefault($this->name) ? $name : $this->name,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ability to change the name.
Example:

public function update(
        #[CookieParameter] string $cookie,
        // with custom name
        #[HeaderParameter(name: 'test-header')] ?string $header,
    ) {}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought I mentioned that adding all sorts of shortcut versions is a slippery slope. It's a lot of classes for a small amount of syntactic sugar.

If we add them, then yes, there will need to be annotation versions as well.

Refactoring to merge annotations and attributes is not really possible without major BC issues.
For one thing constructors would need to match and that just doesn't add up.

Copy link
Contributor Author

@ruscon ruscon Dec 23, 2022

Choose a reason for hiding this comment

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

I'm just making these changes in a separate branch, to see how it behaves. In general, it turns out, but the main problem is that need to switch to a new version of the Annotation format. Here is the documentation. So, need to change a lot of examples :D in the repo. Need to find a way to automate this.

Copy link
Contributor Author

@ruscon ruscon Dec 27, 2022

Choose a reason for hiding this comment

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

When working with method parameters, this is extremely necessary, because you won't be able to name the variables the same as the name of the cookie or header

@ruscon ruscon force-pushed the patch-1 branch 2 times, most recently from 8ddf08c to d851580 Compare December 27, 2022 08:48
@ruscon
Copy link
Contributor Author

ruscon commented Dec 28, 2022

@DerManoMann Did you have a chance to check the PR?
I added annotation classes.
Locally all tests passed successfully.

Copy link
Collaborator

@DerManoMann DerManoMann left a comment

Choose a reason for hiding this comment

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

Looking good.

I wasn't sure we need all those specialized parameter classes, but since I started it with adding PathParameter I suppose it makes sense to add them all...

@DerManoMann DerManoMann merged commit 34aeee1 into zircote:master Jan 4, 2023
@DerManoMann
Copy link
Collaborator

Thanks @ruscon

@ruscon ruscon deleted the patch-1 branch January 4, 2023 21:20
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