EDIorg / ecocomDP

A dataset design pattern and R package for ecological community data.
https://ediorg.github.io/ecocomDP/
Other
32 stars 13 forks source link

Fix issue with single allow element #140

Closed kzollove closed 2 years ago

kzollove commented 2 years ago

Issue originally raised in https://github.com/EDIorg/hymetDP/issues/15 and fixed in https://github.com/EDIorg/hymetDP/commit/de0fa162bc4a9398bd6cbac252c82b56f5c8202b.

clnsmth commented 2 years ago

@kzollove I can't reproduce this result. Can you please verify that this is an issue in ecocomDP? What is your sessionInfo()?

kzollove commented 2 years ago

@clnsmth Its a potential issue that at this point I apparently can not verify.

My attempt to verify is to upload an EML with only one to staging and run create_eml() with that as the source. Since PASTA automatically adds access for the EDI user, only an LTER site (knb-lter-xxx) can upload EML with single (from what I can tell).

Ultimately, its an edge case that I never before ran into with ecocomDP but just now have with hymetDP. Since the create_eml() scripts are nearly identical, I assumed it could crop up in a future ecocomDP package.

clnsmth commented 2 years ago

@kzollove I am able to reproduce this issue with https://pasta.lternet.edu/package/metadata/eml/knb-lter-jrn/210437020/28

clnsmth commented 2 years ago

@kzollove This issue is arising from the inconsistent way in which EML::read_eml() represents EML nodes containing 1 child and >1 child. In the case of knb-lter-jrn.21043.28 there is only one child of /eml/access/allow resulting in

$principal
[1] "public"

$permission
[1] "read"

When >1 child of /eml/access/allow, as in the case of knb-lter-hfr.118.33 EML::read_eml() results in

[[1]]
[[1]]$principal
[1] "uid=HFR,o=lter,dc=ecoinformatics,dc=org"

[[1]]$permission
[1] "all"

[[2]]
[[2]]$principal
[1] "public"

[[2]]$permission
[1] "read"

Your suggested fix works

if (length(unlist(eml_L0$access$allow)) == 2) { # prevent single allow issue
    eml_L0$access$allow <- list(eml_L0$access$allow,eml_L0$access$allow)
}

but am cleaning up a little and would like it implemented as

# A patch for EML::read_eml() handling of nodes with 1 child vs > 1 child
  if (!is.null(names(eml_L0$access$allow))) {
    eml_L0$access$allow <- list(eml_L0$access$allow)
  }

Could you please refactor your PR accordingly? I'd like to retain the link to this PR/issue.

P.S. @kzollove many thanks for identifying this bug!

clnsmth commented 2 years ago

Thanks @kzollove