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

refactor(proposal): use openapi plugin #1629

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Conversation

emigun
Copy link
Contributor

@emigun emigun commented Jan 20, 2025

Description

Use openapi plugin in proposal module

Motivation

Less clutter in code and less risk for inconstensies between openapi spec and the actual behaviour.

Fixes

Related to #1590

Changes:

Removed all @ApiProperty decorators in proposal module.

The first commit doesn't change the openapi spec. The second commit, which touches three fields, does make some changes which we can discuss.

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

@emigun emigun requested a review from nitrosx January 20, 2025 08:14
@emigun emigun self-assigned this Jan 20, 2025
@emigun emigun requested a review from Junjiequan January 20, 2025 09:23
@emigun
Copy link
Contributor Author

emigun commented Jan 20, 2025

There are two commits. The first commit doesn't change anything in the swagger spec. The second one does, for proposal.schema.ts.

For field metadata:
Is it on purpose that the type in ApiProperty is MongooseSchema.Types.Mixed? Seems more reasonable to just have object , for the api users. Also added a default value.

For field parentProposalId :
Changed to string | null since it is nullable, I set the default value to null. Also, changed to optional field because it has a default anyway? What would the point be of having a default if it is required?

For field type:
Same here, changed to optional field since there is a default and I set the default. It is of course possible to only have the default in the ApiProperty, but probably not any real practical difference on setting it?

Copy link
Member

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

For field metadata:

Is it on purpose that the type in ApiProperty is MongooseSchema.Types.Mixed? Seems more reasonable to just have object , for the api users. Also added a default value.

I do not know why we set it to this type. I agree that using Object is easier. Could you please try switching it to object and verify that all the test pass?

For field parentProposalId :

Changed to string | null since it is nullable, I set the default value to null. Also, changed to optional field because it has a default anyway? What would the point be of having a default if it is required?

OK for the type. I would make required in the schema as you have the default value. It is optional in the dto.

For field type:

Same here, changed to optional field since there is a default and I set the default. It is of course possible to only have the default in the ApiProperty, but probably not any real practical difference on setting it?

Same as parentPRoposalId, optional in the dto, but required in the schema set to the default value.

@emigun
Copy link
Contributor Author

emigun commented Jan 20, 2025

For field metadata:

Is it on purpose that the type in ApiProperty is MongooseSchema.Types.Mixed? Seems more reasonable to just have object , for the api users. Also added a default value.

I do not know why we set it to this type. I agree that using Object is easier. Could you please try switching it to object and verify that all the test pass?

It is already object since I removed the ApiProperty decorator. The openapi plugin will read the Record type.

For field parentProposalId :

Changed to string | null since it is nullable, I set the default value to null. Also, changed to optional field because it has a default anyway? What would the point be of having a default if it is required?

OK for the type. I would make required in the schema as you have the default value. It is optional in the dto.

I have looked around a bit on the web and it seems the normal way is to not use required when you have a default.

parentProposalId and type are a bit different in that parentProposalId had required: false in the Prop but type had required: true in the Prop decorator. None of them had a ? on the field. Perhaps just drop required on both of them? Without a ? on the property it should be in the required list in the swagger.

@emigun emigun force-pushed the openapi-plugin-proposals branch 2 times, most recently from 20b405f to e1fd523 Compare January 22, 2025 07:19
@emigun emigun marked this pull request as ready for review January 22, 2025 07:24
@emigun emigun force-pushed the openapi-plugin-proposals branch 2 times, most recently from 4f9b12f to 116a49c Compare January 22, 2025 12:45
@emigun emigun enabled auto-merge January 22, 2025 12:46
@emigun emigun disabled auto-merge January 22, 2025 12:46
@emigun emigun enabled auto-merge (squash) January 22, 2025 12:47
@emigun emigun force-pushed the openapi-plugin-proposals branch from 116a49c to 316c6fb Compare January 22, 2025 12:57
@emigun emigun force-pushed the openapi-plugin-proposals branch from 316c6fb to 81778ad Compare January 22, 2025 21:32
@emigun emigun merged commit adaacd0 into master Jan 22, 2025
13 checks passed
@emigun emigun deleted the openapi-plugin-proposals branch January 22, 2025 21:42
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