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

Fix controller inheritance #339

Merged
merged 8 commits into from
Oct 4, 2021
Merged

Conversation

DamienSynthesis
Copy link
Contributor

Fix controller inheritance

Description

Controller inheritance at the moment does not work when multiple controllers inherit from the parent controller because the child controller metadata is stored on the parent controller. This means that each of the child controllers will have the endpoints from each of the other children. This PR fixes this by storing the parent controller metadata on the parent, and the child controller metadata on the child. When the controllers are added to express, the parent controller metadata is fetched from the parent and combined with the child controller metadata.

Related Issue

Fixes inversify/InversifyJS#854

Motivation and Context

This issue broke a CRUD generic controller on my project. I then found inversify/InversifyJS#854 and created a fix.

How Has This Been Tested?

The current unit test for controller inheritance has been extended to include multiple child controllers as well as the route info being printed to the console

Types of changes

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [ x ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [ x ] I have read the CONTRIBUTING document.
  • [ x ] I have added tests to cover my changes.
  • [ x ] All new and existing tests passed.

@dcavanagh
Copy link
Member

@DamienSynthesis will you please update this PR and resolve the conflicts? Thank you

Copy link
Member

@dcavanagh dcavanagh left a comment

Choose a reason for hiding this comment

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

LGTM

@dcavanagh dcavanagh merged commit 9a9ac5f into inversify:master Oct 4, 2021
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.

As of 5.2.2 / 6.0.0 Inheriting controllers can lead to a crash instead of a 404.
2 participants