Nelmioapidocbundle: Expression message in pattern

[NicolasGuilloux]

Hi guys,

I was implementing an API with a DTO, and I was using the Symfony's Expression assertion. So I implemented the expression and the error message... Until I notice that the error message is displayed as a pattern in Nelmio.

So the property looks like this

And the result is that

I don't understand why this was implemented in the first place. For me, the message from the Expression is an error message where the pattern is a description of the expected input.

This is where the pattern is added: https://github.com/nelmio/NelmioApiDocBundle/blob/master/ModelDescriber/Annotations/SymfonyConstraintAnnotationReader.php#L72

Thanks in advance for the answers :)

[GuilhemN]

I believe this is just a mistake.
We should use $annotation->expression indeed, PR welcome if you'd like to make the change :)

By the way, this class probably should be reworked to support metadata from all loaders (Yaml and XML notably are missing).

[NicolasGuilloux]

I am currently preparing the PR, but I am still doubtful about the pattern. Is the expression maybe a little bit too technical to be displayed as it is in the documentation?

Or maybe it would be worth to create a configuration enable/disable annotations to let the user choose what should be displayed?

Anyway thanks @GuilhemN :) I'll paste the same interrogations within the PR!

[pratyam]

I believe this is just a mistake.
We should use $annotation->expression indeed, PR welcome if you'd like to make the change :)

By the way, this class probably should be reworked to support metadata from all loaders (Yaml and XML notably are missing).

@GuilhemN @NicolasGuilloux I think we shouldn't write nothing in pattern. Inside pattern is expected a regular expression, writing anything else there too "Symfony specific" does not make any sense to me.

[NicolasGuilloux]

I agree but I think an option to enable/disable it is less restrictive than simply systematically adding/removing the pattern.

I'll dig tomorrow to check if I can add this option. Same for the other constraints. It will be something like following :

nelmio_doc:
    symfony_constraints:
        not_blank: true
        not_null: true
        ...
        expression: false

[pratyam]

I was not talking to removing the pattern, I was suggesting to do nothing, also remove the 3 lines and fix the tests That's the fix for me. The pattern should only be added if you use Assert\Regex. Your idea in the last comment is interesting but that's a different issue in my opinion

[NicolasGuilloux]

I was talking about disabling the part of the code that appends the expression to the pattern, not disabling the pattern itself.

In that case, you can still have the pattern filled using the regex annotation without being parasited by the expression annotation.

If your API is really designed for Symfony's users, it could be great to have it displayed. So I really thing that should be configurable because if I can find a case where it should be displayed, there are hundred of other cases that nobody can think right now :p

[pratyam]

In that case, you can still have the pattern filled using the regex annotation without being parasited by the expression annotation.

If I don't want any "pattern" for my parameter, I can't remove it, I can only override completely the property using the @SWG\Property() annotation.

If your API is really designed for Symfony's users, it could be great to have it displayed. So I really thing that should be configurable because if I can find a case where it should be displayed, there are hundred of other cases that nobody can think right now :p

We are generating a swagger documentation, APIs are meant to decouple programming languages / frameworks. SDKs make API integrations for a particular Framework, not the API itself. Also yo can always add a description if you want you to give insights to the clients of the API.

I don't see need of the configuration but anyway it's a different topic. One topic/issue is fixing this bug which generates an invalid Swagger documentation, another topic is to have an extra configuration like you say. I wouldn't mix both things in the same PR.

Salut! 🇫🇷

[NicolasGuilloux]

Okay I start to get it :p @GuilhemN do you know what should we do? Remove the expression from the pattern as it will never be a regex?

Fun fact: the image did not load on my mobile so I didn't understand that pattern documentation explicitly stated that it should only be Javascript regex. Sorry ^^

[GuilhemN]

According to the specification, pattern actually SHOULD be a regex, so I think it would be fine to use a symfony expression.
But honestly I don't have a strong opinion here, I understand that having @Expression shown in the docs is convenient, and I also understand that you may not want to have those displayed.

Since it doesn't make your docs invalid (SHOULD is used in the specification), I guess adding extra information doesn't hurt.
Does the fact that SHOULD is used make it more acceptable to document @Expression or still not?

[pratyam]

You have to read the whole documentation. This "SHOULD" it doesn't mean that pattern can be a free text to put whatever we want. Read the whole documentation: https://www.ecma-international.org/ecma-262/5.1/#sec-15.10.1

Also look at the swagger specification:

That's why it creates a wrong documentation because the generated client / server will try to apply the rule that you described in "pattern"

Also I don't think that to show an specific code that is only valid for Symfony / PHP is a good idea. But if you still think that we can give this information to the client, it's fine, but this show go inside the "description"

[NicolasGuilloux]

I have no strong opinion about this neither. Actually, I really think that it is up to the developer to enable/disable this feature. I know that if we could, we would disable it because the Expression assertion is a quick and lazy way to avoid writing a discrete constraint and these are not displayed.

That being said, I'm not sure it should be removed. Even if the actual behaviour is not appropriate, it does not mean that somebody does not use it.

Finally, I guess this goes off topic a little bit. This issue and its associated PR was really about the expression instead of the message. Do you mind creating another issue @pratyam so that it will be better labeled and more people will join the discussion?

Cheers guys!

[GuilhemN]

You have to read the whole documentation. This "SHOULD" it doesn't mean that pattern can be a free text to put whatever we want. Read the whole documentation: https://www.ecma-international.org/ecma-262/5.1/#sec-15.10.1

ok, thanks, their "SHOULD" is not translated into facts then :)

I'd say let's remove this information then. Since ->message was used until now, I don't think it was of any help and in my opinion, we can remove it safely.
Moreover, it is easier to re-add this information if actually needed than to remove it.

Thanks to both of you for bringing this up and debating it!

[GuilhemN]

This should be fixed by #1670, thanks again!