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

feat(cli): implemented `safe files add -` to read from stdin #354

Merged
merged 3 commits into from Jan 8, 2020

Conversation

@dan-da
Copy link
Contributor

dan-da commented Dec 8, 2019

This PR enables reading from stdin for safe files add when - is specified as the location.

Example:

$ cat Cargo.toml | safe files add - safe://hnyynyww1tcm4dyoqcqr1t44uewi93xkmrbfqiontba5qyoethb7eucoqsbnc/Cargo.from_stdin.toml
FilesContainer updated (version 2): "safe://hnyynyww1tcm4dyoqcqr1t44uewi93xkmrbfqiontba5qyoethb7eucoqsbnc?v=2"
+  /Cargo.from_stdin.toml  safe://hbyyyydeok89zis7f8ca4ed64men66yo8ochfj8pror43k1xrq5hcxkwyq

and we can verify the content is correct:

$ safe cat safe://hbyyyydeok89zis7f8ca4ed64men66yo8ochfj8pror43k1xrq5hcxkwyq | md5sum
45d45fe602da3e3ba9099ed9579c922a  -
$ md5sum Cargo.toml 
45d45fe602da3e3ba9099ed9579c922a  Cargo.toml

Using - to indicate read from stdin is a common unix convention, eg used by the cat command. However it does preclude uploading a file that is actually named -.

Anyway I figured I would submit the most basic/simple implementation first to see if the feature might be accepted, possibly with some tweaks.

I also looked at supporting stdin for the safe files put command, however that seems a bit trickier to implement as it calls files_container_create api, and there is no corresponding files_container_create_from_raw() api. In the case of the add command, I called files_container_add_from_raw() with the stdin data.

@dan-da dan-da requested a review from bochaco as a code owner Dec 8, 2019
Copy link
Member

bochaco left a comment

I wasn't aware of the -, so if this is popular among commands I think we should be ok with using it here and in any other commands we need to support stdin.

safe-cli/subcommands/files.rs Outdated Show resolved Hide resolved
@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 9, 2019

For files put, what do you expect it to be able to read from stdin?, just a file as well and it creates a files container with just that file?

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 10, 2019

re: files put. Yes exactly. Just a single file. Just to be consistent with usage of add.

re: - : Here's some links where they talk about usage of - in unix commands.

https://unix.stackexchange.com/questions/179811/what-does-dash-mean-in-unix-command-line
https://unix.stackexchange.com/questions/16357/usage-of-dash-in-place-of-a-filename
https://unix.stackexchange.com/questions/41828/what-does-dash-at-the-end-of-a-command-mean
https://unix.stackexchange.com/questions/140522/why-do-some-commands-not-read-from-their-standard-input

In general, reading from stdin is useful when invoking prog b from output of prog a, to avoid need to create a tempfile. Sometimes if writing a very large amount of data, there might not even be enough disk space for a tempfile. Also usefui if prog a is invoking prog b directly it can pass b data via a pipe.

@dan-da dan-da force-pushed the dan-da:files_add_stdin_pr branch from feaa2a4 to 4b8ea22 Dec 10, 2019
@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 10, 2019

Cool, I'll read them, I just wasn't aware of the - but I have used pipes in the past, so it all sounds good. If you want I can try to send a PR to your branch (this same one) with what is required to support the files put, give me a couple of days and if I don't find the time in that period we just ignore that for now.

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 10, 2019

I noticed one oddity worth mentioning, though maybe it doesn't need any solution.

safe files add calls let target = get_from_arg_or_stdin(target, None). So if user were to issue command like:

$safe files add -

Then the code will first attempt to read target from stdin (single line read) and then the rest of the stdin data (to eof).

That works technically, but is ... kinda odd.

So long as the target is specified as an arg, then no weirdness. eg this form:

safe files add - safe://...

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 10, 2019

I could do the files put as a separate PR once you add support in safe_api. Or however you think best.

@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 10, 2019

Yes I realise what you say also, but if we are happy with - being the way we specify we want to read from stdin we can then change it so you have to do safe files add - - ? ...I'm not saying it's much better tbh...but dunno..just adding it to the list of options/ideas

@bochaco bochaco added this to In progress in Data types API and CLI commands via automation Dec 10, 2019
@bochaco bochaco added the files cmd label Dec 10, 2019
@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 10, 2019

I guess safe files add - - is a possibility, though the double-read from stdin oddness still exists in that case. If we want to fully avoid that, we could just make target a required arg and not call get_from_arg_or_stdin() But then, i don't know the history/logic of why it is that way to begin with.

@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 10, 2019

Yes that's definitely an option, and the history of that it's just that we originally envisioned what you are starting to do here, support piping commands, you can see we also imagined all this type of things like this in the "Piping commands" section of this post https://safenetforum.org/t/safe-cli-high-level-design-document/28690

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 11, 2019

yeah I just saw that yesterday about piping target around. So if we remove the target stdin read, it breaks that chaining, and is inconsistent both with spec and other subcommands. seems bad.

Other possibilites are:

  • leave as is. both can be read from stdin: target url, then data.
  • throw an error if - is given for location and target arg is not present.

I don't have a strong opinion about it, so just let me know which you think best.

@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 11, 2019

Perhaps we can leave as is for now, I think we need to think about stdin strategy more broadly across all commands so we can have them to be consistent and homogeneous. We'll need to think which are the most important arguments that'll be used for stdin input, and for the rest define how you can choose to provide it from stdin. The answer could simply be "well...we allow one or many args of the same command to be passed from stdin but only one at a time", which is your option two from above I think. Or we can define that maximum one arg can be pass with stdin and the rest need to be worked out using xargs or something similar the user decides to use, in which case I'd like to define again something consistent, e.g. "all commands are allowed to receive from stdin only the target location" or "only the source location", etc.

Having this consistency is important to create the pattern in users' heads, even if they use a command they never used before, they automatically will guess which args can or cannot be passed with stdin

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 11, 2019

I think that in general, cli users (especially on linux/unix) expect that stdin will read the main data that is expected to be processed, which in the case of files put/add is the data to be uploaded.

At the same time, the way that the cli is spec'd, there is a consistency/pattern in the usage of passing target via stdin.

So we have a conflict between these two paradigms and look for a way to resolve it.

I think that reading multiple args from stdin is non-standard and confusing. So I hesitate to go that route. I'm not even sure what the bash syntax for that would be when eg chaining targets.

I think that standardizing on one-or-the-other-but-not-both is a sensible approach. So user can either read raw/file data from stdin, or target, but not both.

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 11, 2019

also, for the sake of consistency, I think it does make sense to recognize - as stdin for the target arg also.

I tried to add it to get_from_arg_or_stdin() but I can't figure out the correct match syntax to avoid duplicating code. What I want to do is something like:

    match target_arg {
        Some("-") | None => {
            ...
        }
        Some(t) => Ok(t),
    }

The compiler doesn't like this (or variants I tried) and there is no fall-through (like C case statement without break) so I'm having difficulty making it concise. any suggestion?

@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 12, 2019

read raw/file data from stdin, or target, but not both

Agreed, I think this is the way to go, otherwise it's not only complex and confusing, but useless for piped commands. The only problem (implementation wise) is that we'll need to add if/else in our code for this, as I don't think structop gives us a way of putting this type of conditions on args which are optional? if so, perhaps we can even work on a PR for structop...maybe not for short term but for medium term so we can remove the conditionals all over the place from CLI afterwards.

I tried to add it to get_from_arg_or_stdin() but I can't figure out the correct match syntax to avoid duplicating code.

I think it's better if we create a parser we use in structop, so we parse it and return None if it matches - so the rest of the code is agnostic and get_from_arg_or_stdin will see a None. You can see how a parser is set here: https://github.com/maidsafe/safe-api/blob/master/safe-cli/subcommands/keys.rs#L58 , the parser itself is in the helper.rs: https://github.com/maidsafe/safe-api/blob/master/safe-cli/subcommands/helpers.rs#L87-L90

@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 13, 2019

I don't see how to return None from a custom parser.

The function signatures for custom parsers are listed here:
https://docs.rs/structopt/0.3.5/structopt/#custom-string-parsers

@dan-da dan-da force-pushed the dan-da:files_add_stdin_pr branch from 4b8ea22 to 6956184 Dec 13, 2019
@dan-da

This comment has been minimized.

Copy link
Contributor Author

dan-da commented Dec 13, 2019

Ok, I updated the PR.

In brief, add - - now prints an error and add - displays a usage message.

As per previous comment, I could not find any way to return None from the parser callback, so I settled for having it return empty string for both - and "".

The arg handling code then checks for "" and treats that to mean read-from-stdin. So technically, from the command-line add "" and add - are equivalent, though the help text only mentions -.

I used requires_if on the location arg to indicate that target is required if location is - or "". This is presently a bit ugly and looks like:

            raw(requires_if = "\"\", \"target\""),
            raw(requires_if = "\"-\", \"target\"")

If/when we upgrade to structopt 0.3.x, this can be:

            requires_if( "", "target"),
            requires_if( "-", "target"),

Unfortunately it seems the requires_if is evaluated by clap/structopt before the parse(), otherwise only the first requires_if would be needed. But instead both are.

I could not find any mechanism in clap/structopt for validating values of two args at once, so I do the check to prevent add - - in the code that handles location arg.

All together, I feel like it could/should be more elegant, but clap/structopt are fighting me. I'm open to concrete suggestions for further improvement.

@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 16, 2019

I'll try to get the parser to return an Option, give me a few hours and I'll be either sending a PR to your branch or sharing my findings, I think I've done it before so let's see.

The requires_if seems promising, let me look at it too, we can upgrade structopt now with this PR BTW.

@bochaco

This comment has been minimized.

Copy link
Member

bochaco commented Dec 17, 2019

@dan-da I sent dan-da#1 , please take a look at it.

@dan-da dan-da requested a review from maidsafe/frontend_codeowners as a code owner Jan 8, 2020
@dan-da dan-da force-pushed the dan-da:files_add_stdin_pr branch 2 times, most recently from 2101264 to d3df202 Jan 8, 2020
@dan-da dan-da force-pushed the dan-da:files_add_stdin_pr branch from 7b920b7 to 6b69635 Jan 8, 2020
@bochaco
bochaco approved these changes Jan 8, 2020
@bochaco bochaco merged commit e608a0a into maidsafe:master Jan 8, 2020
16 checks passed
16 checks passed
Rustfmt-Clippy
Details
Build FFI Android (armv7-linux-androideabi, safe-ffi)
Details
Build FFI Android (x86_64-linux-android, safe-ffi)
Details
Build FFI iOS Build FFI iOS
Details
Test Component (ubuntu-latest, api-tests)
Details
Test Component (ubuntu-latest, cli-tests)
Details
Test Component (ubuntu-latest, e2e-authd-mock-tests)
Details
Test Component (ubuntu-latest, e2e-authd-vault-tests)
Details
Test Component (windows-latest, api-tests)
Details
Test Component (windows-latest, cli-tests)
Details
Test Component (windows-latest, e2e-authd-mock-tests)
Details
Test Component (windows-latest, e2e-authd-vault-tests)
Details
Test Component (macos-latest, api-tests)
Details
Test Component (macos-latest, cli-tests)
Details
Test Component (macos-latest, e2e-authd-mock-tests)
Details
Test Component (macos-latest, e2e-authd-vault-tests)
Details
Data types API and CLI commands automation moved this from In progress to Done Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.