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

Return errors on invalid entity paths #3393

Closed
emilk opened this issue Sep 21, 2023 · 1 comment · Fixed by #3443
Closed

Return errors on invalid entity paths #3393

emilk opened this issue Sep 21, 2023 · 1 comment · Fixed by #3443
Labels
🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start
Milestone

Comments

@emilk
Copy link
Member

emilk commented Sep 21, 2023

We currently have this in our code:

impl From<&str> for EntityPath {
    #[inline]
    fn from(path: &str) -> Self {
        path.parse().unwrap()
    }
}

impl From<String> for EntityPath {
    #[inline]
    fn from(path: String) -> Self {
        path.parse().unwrap()
    }
}

This means that passing an invalid entity path (e.g. ///åmål! ) to Rerun can crash the SDK, viewer, or both.

We should remove these and return errors on invalid paths instead.

@emilk emilk added enhancement New feature or request 👀 needs triage This issue needs to be triaged by the Rerun team 🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start and removed 👀 needs triage This issue needs to be triaged by the Rerun team enhancement New feature or request labels Sep 21, 2023
@emilk emilk added this to the 0.10 C++ milestone Sep 21, 2023
@emilk
Copy link
Member Author

emilk commented Sep 25, 2023

Another solution to this is to allow anything as an entity path, and just doing our best to parse and interpret it, and perhaps log a warning if does not follow our preferred style.
For instance, /// ///foo/Hallå där.Text would be interpreted as " "/foo/"Hallå där.Text"

emilk added a commit that referenced this issue Sep 25, 2023
Closes #3393

For the Python and Rust SDK, any string is now a valid entity path,
though perhaps not the one you where expecting. A warning is printed
if the format is not the normalized format.

For C/C++ I opted to still use the strict parsing for now.
We can revisit that later.
emilk added a commit that referenced this issue Sep 25, 2023
Closes #3393

For the Python and Rust SDK, any string is now a valid entity path,
though perhaps not the one you where expecting. A warning is printed
if the format is not the normalized format.

For C/C++ I opted to still use the strict parsing for now.
We can revisit that later.
@emilk emilk self-assigned this Sep 25, 2023
emilk added a commit that referenced this issue Sep 25, 2023
Closes #3393

For the Python and Rust SDK, any string is now a valid entity path,
though perhaps not the one you where expecting. A warning is printed if
the format is not the normalized format.

For C/C++ I opted to still use the strict parsing for now. We can
revisit that later.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3443) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3443)
- [Docs
preview](https://rerun.io/preview/2d8c818b7175d8daec5149ff1bb4442a1501e5f3/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/2d8c818b7175d8daec5149ff1bb4442a1501e5f3/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
@emilk emilk removed their assignment Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant