-
Notifications
You must be signed in to change notification settings - Fork 464
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
fix(plugin): include property if using @Matches with chained expression #2747
base: master
Are you sure you want to change the base?
fix(plugin): include property if using @Matches with chained expression #2747
Conversation
dc262b3
to
ec17c65
Compare
can you show an example of how the swagger UI would look like? |
Given below model: export const DemoValidation = {
a: {
b: /^abc$/,
c: { d: /^abc$/ },
e: [/^abc$/],
},
};
const PATTERN = /^abc$/;
export class CreateDemoDto {
@IsString()
@Matches(DemoValidation.a.b)
name: string;
@IsString()
@Matches(DemoValidation.a.c.d)
name_1: string;
@IsString()
@Matches(DemoValidation.a.e[0])
name_2: string;
@IsString()
@Matches(/^abc$/)
name_3: string;
@Matches(PATTERN)
@IsOptional()
name_4: string;
} |
I like the ideia but it also feels like we are leaking internal info Can't we expose the regex (ie, the pattern itself) instead of a 'label'? 🤔 |
Running into same issues that this PR fixes. Thanks for the all the work guys |
@micalevisk @quangtran88 are there any remaining to-dos to get this merged besides resolving conflicts ? |
ec17c65
to
8ba5b04
Compare
I have successfully resolved the conflicts, thanks to @okonon. Upon further investigation, I discovered that this plugin generates Swagger metadata during the compilation of the source code. Consequently, obtaining the actual value at this stage is not feasible. I believe the focus for addressing this issue should be on fixing the missing property error. As for the new feature proposed by @micalevisk, it requires a more in-depth investigation and additional effort. It would be advisable to open a separate issue for this. What are your thoughts on this, @micalevisk? |
A new issue to report that seems good |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What is the current behavior?
When utilizing the
@Matches
decorator with a chained expression as the first argument, the associated property is not included in the_OPENAPI_METADATA_FACTORY
. Consequently, this omission leads to the property being absent in the generated Swagger schema.Issue Number: #2723
What is the new behavior?
The property decorated with
@Matches
with a chained expression as the first argument, should now be properly defined in the generated Swagger schema. This resolves the issue of missing properties in the Swagger documentation.Does this PR introduce a breaking change?
Other information
The root cause lies in the following line:
swagger/lib/plugin/visitors/model-class.visitor.ts
Line 673 in 33bec34
When the first argument is a chained expression,
head(decoratorArguments).text
resolves toundefined
. To address this issue, we can use the getText() method for a safer extraction of the pattern information passed in the first argument. For example:@Matches(a.b.c)
will resolve as"a.b.c"
(instead ofundefined
using.text
)@Matches(a)
will resolve as"a"
(same as using.text
)@Matches(/^[+]?abc$/)
will resolve as"/^[+]?abc$/"
(same as using.text
)