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

KV should raise an error if the key contains unacceptable characters #553

Open
kevinhikaruevans opened this issue Apr 8, 2024 · 3 comments · May be fixed by #571
Open

KV should raise an error if the key contains unacceptable characters #553

kevinhikaruevans opened this issue Apr 8, 2024 · 3 comments · May be fixed by #571
Assignees
Labels
accepted The defect or proposal as been accepted defect Suspected defect such as a bug or regression

Comments

@kevinhikaruevans
Copy link

Proposed change

If the key contains unacceptable characters (e.g. whitespace), an error should be raised. Maybe BadSubjectError can be reused or a new error can be created (e.g. BadKeyError).

It currently gives an UnexpectedEOF error and causes a TimeoutError. This is fairly confusing because it doesn't necessarily imply the key is bad and could be remedied by a quick check on the key.

Use case

I had somebody else feeding me some data, which was being fed into nats kv. I noticed that occasionally, an entry would cause an eof error and a timeout error and it wasn't very clear why. I finally realized that the user was putting spaces in the keys, which was causing problems in the underlying publish() call.

Contribution

Sure. I would assume it would be putting in a little check in put()

@kevinhikaruevans kevinhikaruevans added the proposal Enhancement idea or proposal label Apr 8, 2024
@caspervonb caspervonb added accepted The defect or proposal as been accepted defect Suspected defect such as a bug or regression and removed proposal Enhancement idea or proposal labels Jun 11, 2024
@caspervonb
Copy link
Contributor

I'd consider this a defect, we should match the validation that Go does in this case (https://github.com/nats-io/nats.go/blob/main/kv.go#L558)

@caspervonb caspervonb self-assigned this Jun 11, 2024
@caspervonb caspervonb linked a pull request Jun 17, 2024 that will close this issue
@skewty
Copy link

skewty commented Jul 14, 2024

Is this not the Primitive Obsession code smell?

https://refactoring.guru/smells/primitive-obsession

from typing import Final, Self
import re

class Key(str):
    PATTERN: Final = re.compile(r"^[A-Za-z_-]+[A-Za-z0-9_-]*$")  # whatever you want / need

    def __new__(cls, *args, **kwargs) -> Self:
        value = super().__new__(cls, *args, **kwargs)
        # really any checks you require here but likely regex will suffice
        if cls.PATTERN.match(value) is None:
            raise ValueError(f'Key {value} contains invalid characters')
        return value

@skewty
Copy link

skewty commented Jul 14, 2024

IMO these Value classes should be used elsewhere in the code-base as well. Example bucket name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The defect or proposal as been accepted defect Suspected defect such as a bug or regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants