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

Ensure all {modelIdentity}/{action} actions are handled as 'model-related' #82

Merged
merged 15 commits into from
Jun 23, 2020

Conversation

danielsharvey
Copy link
Collaborator

@danielsharvey danielsharvey commented Jun 21, 2020

Overview

Ensure all {modelIdentity}/{action} actions are handled as 'model-related':

  1. All {modelIdentity}/{action} actions treated as related to the model (including /allActions) and Swagger content integrated / merged appropriately.
  2. Actions2 actions recognised as model-related, and therefore recognised as blueprint action overrides (where applicable)
  3. Improve consistency of handling/merging of actions (and Swagger documentation) relating to model controllers.

Details

  • refactor: Improve SwaggerRouteInfo interface

    Improve SwaggerRouteInfo interface to support above:

    1. Add separate fields for action vs blueprint action.
    2. Add separate flag for shortcut blueprint routes.
  • refactor: Refactor route parsing using {modelIdentity}/{action}

    Refactor route parsing to rely predominantly on {modelIdentity}/{action} to ensure all model-related actions (including overrides including actions2) are captured. This is consistent with Sails parseBlueprintOptions().

  • refactor: Refactor transformations phase for SwaggerRouteInfo changes

  • refactor: Refactor Swagger generation for model-related changes

    Refactor generation to improve route/model/controller Swagger merge.

    Refactor for SwaggerRouteInfo changes.

  • test: Refactor tests for Swagger generation (model-related changes)

    Swagger generation changes related to new test cases and model-related merge changes (UserController now equivalent to User model).

    Swagger generation merge changes resulted in ordering changes in generated output.

    Add new Actions2 action overriding blueprint action.

    Adjust UserController to add new Swagger merge.

    Includes full re-generation of parsed routes based on SwaggerRouteInfo changes.

    Minor adjustment to Sails AttributeDefinition as type optional.

Improve `SwaggerRouteInfo` interface to support above:
1. Add separate fields for action vs blueprint action.
2. Add separate flag for shortcut blueprint routes.

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Refactor route parsing to rely predominantly on `{modelIdentity}/{action}`
to ensure all model-related actions (including overrides including
actions2) are captured. This is consistent with Sails
`parseBlueprintOptions()`.

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Refactor generation to improve route/model/controller Swagger merge.

Refactor for `SwaggerRouteInfo` changes.

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Includes full re-generation of parsed routes based on
`SwaggerRouteInfo` changes.

Minor adjustment to Sails AttributeDefinition as `type` optional.

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Add new Actions2 action overriding blueprint action.

Adjust `UserController` to add new Swagger merge.

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Swagger generation merge changes resulted in ordering changes in
generated output.

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Swagger generation  changes related to new test cases and model-related
merge changes (UserController now equivalent to User model).

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
Swagger generation  changes related to new test cases and model-related
merge changes (UserController now equivalent to User model).

Big picture for PR: Ensure all `{modelIdentity}/{action}` actions
are handled as 'model-related':
1. All `{modelIdentity}/{action}` actions treated as related to the
   model (including `/allActions`) and Swagger content integrated /
   merged appropriately.
2. Actions2 actions recognised as model-related, and therefore
   recognised as blueprint action overrides (where applicable)
3. Improve consistency of handling/merging of actions (and Swagger
   documentation) relating to model controllers.
@danielsharvey
Copy link
Collaborator Author

Closes #81.

@danielsharvey
Copy link
Collaborator Author

@theoomoregbee A review would be good for this change. The way {modelIdentity}/{action} actions were parsed meant that actions2 actions which override default blueprint action were not treated as such during Swagger generation. I've reviewed and re-worked this flow. This has resulted in a few other side-effects (fixes, but somewhat subtle) in the way controller vs model swagger is merged. I've checked the resulting changes to the test Swagger generated carefully (you can follow through the individual commits on this) but would be good to have a second opinion.

@theoomoregbee
Copy link
Owner

cool! will give this a look before end of today. Thanks @danielsharvey

Copy link
Owner

@theoomoregbee theoomoregbee left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor comments

lib/parsers.ts Show resolved Hide resolved
lib/swagger-doc.ts Outdated Show resolved Hide resolved
Copy link
Owner

@theoomoregbee theoomoregbee left a comment

Choose a reason for hiding this comment

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

👍

@danielsharvey danielsharvey merged commit 838e997 into master Jun 23, 2020
@theoomoregbee theoomoregbee deleted the fix-custom-blueprint branch June 23, 2020 02:12
@theoomoregbee
Copy link
Owner

🎉 This PR is included in version 3.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants