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

pre-populate required arguments from request body #4827

Merged
merged 2 commits into from
Mar 19, 2018
Merged

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Mar 17, 2018

This way, we can always assume that indexing a required argument works.

Also:

  • test that the command tree doesn't have any obvious bugs (duplicate options, arguments in the wrong order, etc).
  • simplify the usage ParseBodyArgs.

fixes #4823

@Stebalien
Copy link
Member Author

Stebalien commented Mar 17, 2018

This looks bad:

ok 78 - 'ipfs add -r --raw-leaves' output looks good

expecting success: 
    { echo "$MARS"; echo "$VENUS"; } | ipfs cat >actual
  
ok 79 - ipfs cat accept many hashes from built input

expecting success: 
    cat mountdir/planets/mars.txt mountdir/planets/venus.txt >expected &&
    test_cmp expected actual
  
> diff -u expected actual
--- expected	2018-03-17 21:04:04.055381963 +0000
+++ actual	2018-03-17 21:04:04.043382082 +0000
@@ -1,2 +1 @@
 Hello Mars!
-Hello Venus!

not ok 80 - ipfs cat output looks good
#	
#	    cat mountdir/planets/mars.txt mountdir/planets/venus.txt >expected &&
#	    test_cmp expected actual
#	  

@Stebalien
Copy link
Member Author

So, the problem here is that the client is reading off one arg and then forwarding that to the server. However, I've replaced Files with nil so we don't forward the rest to the server.

@Stebalien
Copy link
Member Author

So, the question here is, should we check the arguments on the client? Probably but, in that case, we're going to be splitting them (passing one via the query string and the rest via the body).

@whyrusleeping
Copy link
Member

we're going to be splitting them (passing one via the query string and the rest via the body).

This actually seems reasonable. It moves us towards having the requirement that all required arguments are specified via the query string, and the body is only used for optional/variadic args

@whyrusleeping
Copy link
Member

However, I've replaced Files with nil so we don't forward the rest to the server.

Wait up, i must have missed this in the CR. We should just replace files with the next file.

@Stebalien
Copy link
Member Author

Stebalien commented Mar 18, 2018

Blocked on ipfs/go-ipfs-cmds#83

@Stebalien Stebalien force-pushed the fix/4823 branch 2 times, most recently from ce316e5 to 9837f0e Compare March 18, 2018 19:48
This way, we can always assume that indexing a required argument works.

Also:

* test that the command tree doesn't have any obvious bugs (duplicate options,
  arguments in the wrong order, etc).
* simplify the usage ParseBodyArgs.
* remove unnecessary check in the get command.

fixes #4823

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
for both online and offline mode (tests both the cli and the api)

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
grep "not enough arugments provided" curl_out


grep "argument \"ipfs-path\" is required" curl_out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats a much better error message.

@Stebalien Stebalien added RFM and removed status/in-progress In progress labels Mar 19, 2018
@whyrusleeping whyrusleeping merged commit 4bdbe1a into master Mar 19, 2018
@whyrusleeping whyrusleeping deleted the fix/4823 branch March 19, 2018 05:19
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
pre-populate required arguments from request body

This commit was moved from ipfs/kubo@4bdbe1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic in handler
3 participants