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: Use server config for direct bindings #768

Closed
wants to merge 1 commit into from

Conversation

JorTurFer
Copy link

Using direct bindings, the consumer shouldn't need to know about the already configured consumer in the server.
This PR overrides user given configuration (if given) with server received configuration in cases where a direct binding is set.

@JorTurFer JorTurFer changed the title fix: Use server config for direct bindings WIP - fix: Use server config for direct bindings May 11, 2023
@JorTurFer JorTurFer changed the title WIP - fix: Use server config for direct bindings fix: Use server config for direct bindings May 11, 2023
@scottf
Copy link
Collaborator

scottf commented May 11, 2023

Even with bind and the fact that we will always use the server version, that it does not prevent the user from providing a consumer config or a subscribe subject that does not match the server consumer config. When we did not validate, there were times when we would get support questions of why am I getting these subjects, for instance getting all subjects because the filter subject was > when I only used foo in the subscribe api call.
If the api were better and allowed the dev to simply supply the stream and consumer name, it would be acceptable to just skip the validation. In fact this is an important part of our simplification project.

@JorTurFer
Copy link
Author

JorTurFer commented May 11, 2023

The problem here is that the backoff isn't a valid variable thing. I can update the PR to allow it as variable things, but I think that validate that user isn't providing any extra data than stream name and consumer name could be the best approach.

Somthing like, if you have already set any other value than stream and consumer and you set binding, the code will raise an error (also in other orders like binding first and then other values, etc). In that case, we could give feedback to the users about the problem during the application startup (something like: "Direct binding can't be set if you have already set this property", or, "When direct binding only stream and consumer name can be set", etc) and also we could simplify the code just using the skipping as I have done.

Could this be a solution? It would provide feedback to users about how they should use the client and also prevent the code from these cases of mismatches because we could use the server config

@JorTurFer JorTurFer closed this May 12, 2023
@JorTurFer
Copy link
Author

I close this PR on favor of https://github.com/nats-io/nats.net/pull/769

@JorTurFer JorTurFer deleted the fix-direct-binding branch May 12, 2023 07:11
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