insightsengineering / teal.slice

Reproducible slice module for teal applications
https://insightsengineering.github.io/teal.slice/
Other
11 stars 5 forks source link

[Bug]: The filter panel feature does not work well when data/vars have special characters #570

Open vedhav opened 4 months ago

vedhav commented 4 months ago

What happened?

The namespace creation function for the data and column names can be more robust. Right now the special characters are stripped from the name space which might not be ideal.

library(teal)

app <- init(
  within(teal_data(), {
    iris <- iris
    iris[["Species!"]] <- iris$Species
    iris[["Species!@#$%^&*_-+={[()]}:;,<>./?"]] <- iris$Species
  }),
  modules(
    example_module("Module 1"),
    example_module("Module 2"),
    modules(
      label = "Nested modules",
      example_module("Module 3"),
      example_module("Module 4"),
      modules(
        label = "Sub nested modules",
        example_module("Module 5"),
        example_module("Module 6")
      )
    )
  ),
  filter = teal_slices(
    teal_slice(dataname = "iris", varname = "Species", multiple = FALSE),
    teal_slice(dataname = "iris", varname = "Species!", multiple = TRUE),
    teal_slice(dataname = "iris", varname = "Species!@#$%^&*_-+={[()]}:;,<>./?", multiple = TRUE),
    teal_slice(dataname = "iris", varname = "Sepal.Length", multiple = TRUE)
  )
)

shinyApp(app$ui, app$server)

sessionInfo()

No response

Relevant log output

No response

Code of Conduct

Contribution Guidelines

Security Policy

chlebowa commented 4 months ago

What are you trying to accomplish here? "Species!@#$%^&*_-+={[()]}:;,<>./?" is not a syntactically valid column name. Also, the issue description is not clear. What is the relationship between a column name and a namespace? How would that string end up in a namespace function?

vedhav commented 4 months ago

Although there is no practical use of having this column. I created this column to showcase the underlying problem. It is possible to pass data with these columns. We have to handle it. We might simply show a warning when such columns are passed, but we need some way to address this.

names(iris)
[1] "Sepal.Length"                      "Sepal.Width"                       "Petal.Length"
[4] "Petal.Width"                       "Species"                           "Species!"
[7] "Species!@#$%^&*_-+={[()]}:;,<>./?"
If you inspect the namespace you'll notice that the namespaces are as follows: Column Name Name-space
Species Species
Species! Species_
Species!!! Species_
Species!@#$%^&*_-+={[()]}:;,<>./? Species_
chlebowa commented 4 months ago

We have to handle it.

No we don't. These are not syntactically valid column names. They wouldn't fly in an interactive session and I see no reason why they should be tolerated in teal.

If you inspect the namespace you'll notice that the namespaces are as follows:

What about Sepal.Width and the others?

vedhav commented 4 months ago

What about Sepal.Width and the others?

Sepal.Width becomes Sepal_Width. Evidently the special character combination is converted to _ So Sepal..Width also becomes Sepal_Width

chlebowa commented 4 months ago

So full stops are not JS friendly, which is why they are converted to underscores. I don't see why duplicates aren't maintained. There is an improvement to be made here but I don't think we should strive to handle ridiculous stuff.

I think a simple make.names(x, unique = TRUE) followed by gsub(".", "_", x, fixed = TRUE) is quite enough.

pawelru commented 4 months ago

In R it is possible to put garbage names in quotes, e.g. things like: df$`-a` <- 1. This will accept anything.

I think that the underlying issue is the fact that this names most likely ends up being a HTML ID string. Just because of that there might be column names validation down the way from user inputs to actuall app HTML creation.

I think we saw issues about that already but can't find these now.

The ultimate solution would be to replace creating IDs from column names with some hashing functionality that will be (i) unique and (ii) valid string accepted as an ID. I think that any gsub based solution can be challenged once you know the pattern and replacement :) Note that this would most likely break many shinytest2 tests because it's a common practice to relay on ID of gui elements.

The other aspect is how likely it is. Here I agree with @chlebowa that this probably needs improvement but IMHO it's quite rare. You should not do df$`<incorrect name>` type of stuff as most likely it's not only teal that would complain

kpagacz commented 4 months ago

I don't think anything would complain about df$<incorrect name> as long as it doesn't end up being an id of an HTML element.

The fact of the matter is, in R, anything inside `` is a valid identifier according to the language specification.