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

[FIXED] KV use of JS prefix #910

Merged
merged 2 commits into from Feb 16, 2022
Merged

[FIXED] KV use of JS prefix #910

merged 2 commits into from Feb 16, 2022

Conversation

kozlovic
Copy link
Member

If the user creates a JS context with a custom prefix, this needs
to be used for the subject of the Put() and Delete() operations.
This is addressing the architecture design ADR-19.

Signed-off-by: Ivan Kozlovic ivan@synadia.com

If the user creates a JS context with a custom prefix, this needs
to be used for the subject of the Put() and Delete() operations.
This is addressing the architecture design ADR-19.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@coveralls
Copy link

coveralls commented Feb 15, 2022

Coverage Status

Coverage increased (+0.3%) to 85.242% when pulling 9bbca0d on kv_cross_accounts into 519bc35 on main.

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

Update is missing

I checked all locations of b.WriteString(kv.pre).

PurgeDeletes uses it but goes through the js api, hence does not count. (as js uses the prefix)
Same applies to Watch.

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

Never Mind, update also goes through js.
LGTM

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

@matthiashanel Actually, you were right. Adding the test showed that the update would fail, so we need to use the prefix there too. I added the test for that.

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic merged commit 0096b1b into main Feb 16, 2022
@kozlovic kozlovic deleted the kv_cross_accounts branch February 16, 2022 00:06
thomastaylor312 added a commit to thomastaylor312/nats.rs that referenced this pull request Apr 22, 2022
This is essentially the same fix as nats-io/nats.go#910
in the go client, complete with test to make sure things work
Jarema pushed a commit to nats-io/nats.rs that referenced this pull request Apr 25, 2022
This is essentially the same fix as nats-io/nats.go#910
in the go client, complete with test to make sure things work
Jarema pushed a commit to nats-io/nats.rs that referenced this pull request Apr 25, 2022
This is essentially the same fix as nats-io/nats.go#910
in the go client, complete with test to make sure things work
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.

None yet

4 participants