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

Refactoring PipelineBehavior as decorator pattern #31

Closed
rozturac opened this issue Oct 13, 2022 · 5 comments
Closed

Refactoring PipelineBehavior as decorator pattern #31

rozturac opened this issue Oct 13, 2022 · 5 comments
Labels
breaking-change enhancement New feature or request

Comments

@rozturac
Copy link
Contributor

rozturac commented Oct 13, 2022

I woould suggest refactoring the PipelineBehavior to be the decorator pattern. After this arrangement, I think it will provide the following benefits.

  1. Instead of handling exception on each pipeline, we can give this responsibility to the last pipeline.
  2. We can set up DistributedLock in our pipeline and interrupt it from going to command (We can do this without throwing an exception)
  3. It allows us to access and manipulate the request and response information over the pipe.

By the way, if you agree, I can do this development with pleasure.

@rozturac rozturac changed the title Refactlr Refactoring PipelineBehavior As Decorator Pattern Oct 13, 2022
@rozturac rozturac changed the title Refactoring PipelineBehavior As Decorator Pattern Refactoring PipelineBehavior as decorator pattern Oct 13, 2022
@osoykan
Copy link
Collaborator

osoykan commented Oct 14, 2022

Hi, thanks for the subject, but it is not clear.

Instead of handling exception on each pipeline, we can give this responsibility to the last pipeline.

Could you elaborate on that? It is too vague. Maybe provide an example of your thought as a comment so we can discuss it.

We can set up DistributedLock in our pipeline and interrupt it from going to command (We can do this without throwing an exception)

This is not the library's responsibility to know any application logic. Hence we shouldn't add any behavioral change for this purpose.

It allows us to access and manipulate the request and response information over the pipe.

The current pipeline already gives this possibility.

@rozturac
Copy link
Contributor Author

rozturac commented Oct 14, 2022

Hi, firstly thanks for the response.

I'll try to explain the improvement I'd like to do with the following pseudo code:

Act -> Func (Request) -> Response
Pipe -> Func (req Request, next Act)
Command -> Act

FirstPipe : Pipe {
 try {
    return next(req)
 } catch (ex Exception){
    //we can handle the exception and return anything
 }
}

SecondPipe : Pipe {
    // we can log request
    result = next(req)
    // we can log response with duration
    return result
}

ExecutionResult -> FirstPipe(SecondPipe(Command))

By the way, it seems that the response value is not coming in as-is pipeline.

@osoykan
Copy link
Collaborator

osoykan commented Oct 17, 2022

I understand what you mean now, It is a good addition; if you would like to add it, could you please target the v2.0 branch?

Also, another PR #34 touches the same area that you want to develop; that PR might be obsolete after the pipeline development.

If you open a PR we can discuss the changes from there.

@rozturac
Copy link
Contributor Author

Hi thanks for your suggestions. Then, I'll do the development on the 2.0 target and open the PR request as soon as possible.

@rozturac
Copy link
Contributor Author

Thanks for your issue support and suggestions 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants