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

Revamp middleware defensive check on arguments #536

Closed
mantzas opened this issue Aug 18, 2022 · 3 comments
Closed

Revamp middleware defensive check on arguments #536

mantzas opened this issue Aug 18, 2022 · 3 comments
Assignees
Labels
breaking bad The feature will introduce a breaking change. enhancement New feature or request

Comments

@mantzas
Copy link

mantzas commented Aug 18, 2022

Is your feature request related to a problem? Please describe

Right now all middleware that take argument do not check if they are valid.
We should check if the arguments are valid and return an error.
This will introduce a breaking change.

@mantzas mantzas added enhancement New feature or request breaking bad The feature will introduce a breaking change. labels Aug 18, 2022
@mantzas mantzas added this to the v1.0.0 Release milestone Aug 18, 2022
@pkritiotis
Copy link
Collaborator

We need to decide how we'll be approaching this in terms of backward compatibility here

  1. Are we okay with introducing this breaking change in the existing package?
  2. Or should we introduce a V2 package under the existing middleware package to introduce this change and keep it backward compatible? (similarly with the big transition of components to V2)

Considering that

  • this is probably an enhancing and not very aggressive change,
  • it probably has a small impact since not many users directly use these middleware
  • it's quick and easy to apply the error checking in case of an upgrade and
  • from a maintenance perspective, we will have one less outdated, less-secure package,

I would lean towards approach (1) introducing a breaking change in the existing package.

I have by no means a strong opinion on this one though

@mantzas wdyt?

@mantzas
Copy link
Author

mantzas commented Aug 19, 2022

I agree!

@pkritiotis
Copy link
Collaborator

Great, we can proceed with this decision then.

I will get assigned to work on this one!

@pkritiotis pkritiotis self-assigned this Aug 19, 2022
mantzas added a commit that referenced this issue Jan 13, 2023
## Which problem is this PR solving?
Resolves Revamp middleware defensive check on arguments #536

## Short description of the changes

Added validations in http middleware and http component options

Co-authored-by: Panayiotis Kritiotis <p.kritiotis@thebeat.co>
Co-authored-by: Sotirios Mantziaris <s.mantziaris@thebeat.co>
Co-authored-by: Sotirios Mantziaris <smantziaris@gmail.com>
@mantzas mantzas closed this as completed Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking bad The feature will introduce a breaking change. enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants