No-fix validation

[nbenn]

Instead of trying to resolve inconsistencies that might arise from changes upstream in a stack by resetting incompatible values to defaults, we now fail validation and require user feedback to resolve issues.

[DivadNojnarg]

@nbenn We probably should not call validate_field inside initialize_field? See https://github.com/blockr-org/blockr/pull/376/files#diff-8546c791c7ba260a074d6a5270c42f3e71c421dd507effb82059def8f8316d53R63.

This currently returns NULL.

[nbenn]

@DivadNojnarg agreed. I modified initialize_field() and update_field() a bit. Not sure if this is what we want exactly, but it should be better than what we had.

[DivadNojnarg]

@nbenn. One remark about validation_failure: calling rlang::abort causes the app to crash. It should probably be handled differently, with rlang::warn as I did in 981f306. Then on the shiny side, req ensures we stop the evaluation if the validation failed. Finally, the message is extracted and sent to the UI.

[DivadNojnarg]

@nbenn update on where I am.

Currently, in the block server function, we execute the validation which in turns allows to do the computation but only for the current block.

Problem:

An issue that arises when having multiple blocks: assuming a stack with a data block and a select block, both valid. Empty the field from the data block, block 1 becomes invalid but somehow, the block2 server functions is triggered again and block2 appears valid because it still has the old values from the previous data. This fails because the columns don't exist in the new expression.

A piece of info is missing at the stack level.

Possible solution:

From the stack level, we should probably pass the validation state of the previous block and add a check at the top level of the block server function: req(previous_block_valid) so that this server function does not go further.

[nbenn]

@DivadNojnarg many thanks!

Regarding stop() vs warning(), that was done on purpose. If executed in a tryCatch with a handler for validation_failure (as is the case for is_valid.field()), this should not cause the app to crash, cf

is_valid(new_string_field())

throws and catches the error.

The reason for not using warnings is that as soon as a validation failure is encountered, validation stops. This way you can have conditions is a sequence, where the next is only evaluated when the previous passes. An example for what I mean

validate <- function(val, fun) {
  if (length(val) != 1)
    fun("expecting a string")
  if (!val %in% c("foo", "bar"))
    fun("expecting certain values")
  invisible(NULL)
}

validate("foo", stop)
validate(character(), stop)
validate(character(), warning)

With warnings you have no control over the error that is ultimately thrown.

I'll try to find out why the error is not properly handled from within the shiny app. Do you know, is error handling in shiny different from base R?

[DivadNojnarg]

@nbenn Commit e05ff8d fixes the error issue. No crash anymore. Now I need to solve this.

[DivadNojnarg]

@nbenn That's where we currently are. Resetting the data/changing dataset seems to work for basic blocks.

https://github.com/blockr-org/blockr/assets/18291543/fe0a17ed-079a-472a-b53f-843608d13b58

Few issues:

• Selecting multiple values in the select block triggers a validation error. I imagine multiple values are not handled properly.
• The filter/summarise block seem broken (the nested field related to the column does not show).
• As soon as the block has a selected column (in a select block for instance), that's not possible to clear the current choice before selecting another. I would expect the user to be able to empty the list of choices.

[DivadNojnarg]

Digging in the list_field issue, when I create a filter block and putting a browser within ui_update.list_field, I don't see any sub fields:

x
$value
NULL

$sub_fields
character(0)

[[3]]
function(data, columns) {
    determine_field <- function(x) {
      switch(class(x),
        factor = new_select_field,
        numeric = new_range_field,
        new_string_field
      )
    }

    field_args <- function(x) {
      switch(class(x),
        factor = list(levels(x)[1L], choices = levels(x)),
        numeric = list(range(x), min = min(x), max = max(x)),
        list()
      )
    }

    cols <- data[, columns, drop = FALSE]

    ctor <- lapply(cols, determine_field)
    args <- lapply(cols, field_args)

    Map(do.call, ctor, args)
  }
<bytecode: 0x11f2706d0>
<environment: 0x10ce1b440>

attr(,"type")
[1] "literal"
attr(,"class")
[1] "list_field" "field"     
attr(,"title")
[1] "Value"
attr(,"descr")
[1] ""
attr(,"status")
[1] "active"
attr(,"exclude")
[1] FALSE

which means, nothing can be inserted.

[JohnCoene]

Apologies if it is not the best place for comments, let me know if there is a better place.

• Should we export validation_failure? I know users can call rlang::abort() themselves but perhaps this would make it clearer as this is what we use internally + it would enable changing things in that function in the future?
• Maybe picky of me but this PR has blocks following this pattern *_block rather than new_*_block which we used before, should we not do the opposite (just talking about function naming) as this would avoid breaking existing use of blockr

This is the example I have in #378

my_block <- function(...){
  all_cols <- function(data) colnames(data)

  first_col <- \(data) colnames(data)[1]
  second_col <- \(data) colnames(data)[2]

  fields <- list(
    x_var = new_select_field(first_col, all_cols),
    y_var = new_select_field(second_col, all_cols)
  )

  new_block(
    fields = fields,
    expr = quote({
      ggplot2::ggplot(
        data,
        mapping = ggplot2::aes_string(
          x = .(x_var),
          y = .(y_var)
        )
      ) +
        ggplot2::geom_point() +
        ggplot2::geom_line()
    }),
    ...,
    class = "my_block"
  )
}

.S3method("server_output", "my_block", function(x, result, output){
  renderPlot(result())
})

.S3method("uiOutputBlock", "my_block", function(x, ns){
  plotOutput(ns("res"))
})

# we have to implement this
.S3method("generate_server", "my_block", function(...){
  blockr:::generate_server_block(...)
})

stack <-  new_stack(
  new_dataset_block,
  my_block
)

ui <- fluidPage(
  theme = bslib::bs_theme(5L),
  generate_ui(stack)
)

server <- function(input, output, session) {
  generate_server(stack)
}

shinyApp(ui, server)

In "My block" I cannot change any of the inputs, the inputs reset, perhaps I'm not doing this right, or it may be linked to one of the issues pointed out by @DivadNojnarg?

EDIT:

• I see there is a new_submit_field() which certainly is an improvement over adding a class, however it does not work, not sure whether this is known or not.
• It seems the new_filebrowser_field() does not work (it shows an error after initialisation and opening clicking the button to browse a file crashes the app)
• new_range_field() displays a validation on error (expecting a number), changing the range does not fix the issue. Also confused because I'm pretty certain this was a range, hence "range field", where users could select a range (2 pins on the slider), it now only displays one.

[DivadNojnarg]

@nbenn, after digging a bit more, I noticed that get_sub_fields(x) always return NULL, which is an issue as it is used within value<-.list_field:

`value<-.list_field` <- function(x, name = "value", value) {
  set_sub_fields(x, Map(`value<-`, get_sub_fields(x), name, value))
}

This means, Map(value<-, get_sub_fields(x), name, value) does not yield the expected output, so is set_sub_fields.

[DivadNojnarg]

@nbenn regarding the select input validation issue, it is coming from is_string:

is_string <- function(x) {
  is.character(x) && is_scalar(x)
}

When passed multiple values, is_scalar is FALSE.

The select validation is:

validate_field.select_field <- function(x) {
  val <- value(x)
  validate_string(val)
...
}

Initial state, we have character(0) which fails the validation, as expected. Then, as soon as we select a value from the select input, we'd have to apply validate_string to any element (side note: not necessary in Shiny's context as the selectInput does not allow to pass other things than strings in the UI and returns strings):

This would lead to something like:

if (length(val) <= 1) {
    validate_string(val)
  } else {
   # useless in Shiny' context
    lapply(val, validate_string)
  }

The else being "useless" as explained before. Maybe this is still relevant as we plan to initialise blocks outside the stack (outside shiny' context).

[nbenn]

@JohnCoene Yes, validation_failure() should absolutely be an exported function and this is what should be used in block validators. Sorry, this was an oversight on my end and should be fixed now.

[nbenn]

@DivadNojnarg fixed the select field validation (3f5cc7a). Thanks for reporting & debugging.

[nbenn]

@DivadNojnarg one problem with the list field value() and value<-() implementations was that what I had was only correct when manipulating the "value" component and not any other (such as the "sub_fields" component).

Still not quite there yet, as changes are currently being overwritten. Looking into that now.

[nbenn]

@DivadNojnarg I believe the list field issue is now resolved. The value<-() replacement function is super ugly however. Currently the mechanics for these recursive fields is unsatisfyingly complex. We might want to re-think that (but not as part of this PR as unrelated).

[nbenn]

@JohnCoene thanks for pointing out some issues you ran into.

• The new_submit_field() was not added as part of this PR. This is definitely something I want to change, but it is unrelated to this PR: the field is auto-added based on presence of a submit_block class attribute and it currently only works in such blocks (due to how it is integrated in the block server fun). Will tackle this separately.
• The filebrowser field works as part of the filebrowser block (for me at least and with a later state of the code that is). Did your issue maybe come up as part of another block?
• The range field works (as part of the filter block) for me with the current code. What was your context when you ran into an issue?

[JohnCoene]

I see these issues in this block for instance.

test_block <- function(...){
  first <- \() letters[1]
  all <- \() letters

  fields <- list(
    text = new_string_field("default"),
    select = new_select_field(first, all),
    bool = new_switch_field(TRUE),
    numeric = new_numeric_field(1L, min = 0, max = 10L),
    submit = new_submit_field(),
    browser = new_filesbrowser_field(),
    slider = new_range_field(1, 0, 10),
    range = new_range_field(c(1,2), 0, 10)
  )

  new_block(
    fields = fields,
    expr = quote(iris),
    ...,
    class = c("data_block", "my_block")
  )
}