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

Add global option to specify the multibase encoding (server side) #5789

Merged
merged 7 commits into from
Jan 21, 2019

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Nov 22, 2018

Redo of #5464 but this time doing it (mostly) on the server side.

This may not be the most elegant implementation, but for now I think it is the most practical, espacally if we also want to support ?cid-base end points for API requests. We can clean this up later to make things more elegant.

It does all the work in the core/commands package and does not leak into other API's except for the ipfs filestore command where we need to pass an encoder function to FormatLong method.

See #5777 for an alternative client side implementation.

This adds support for --cid-base and --output-cidv1 option to the command line and HTTP API for the following commands:

ipfs add
ipfs files ls
ipfs files stat
ipfs ls
ipfs pin add
ipfs pin ls
ipfs pin rm
ipfs pin verify
ipfs refs
ipfs refs local
ipfs resolve
ipfs tar add
ipfs urlstore add
ipfs filestore dups
ipfs object get
ipfs object put
ipfs object links

Due to the fact that some commands return the CID itself and not a string, some command support the base conversion in text output only until we upgrade to something like refmt which will give us more control over the JSON output:

ipfs filestore ls
ipfs filestore verify
ipfs bitswap stat
ipfs bitswap wantlist
ipfs dag put
ipfs dag resolve

ipfs dag get currently does not support --cid-base due to the way it is implemented. This is considered a bug.

For some commands the original path string is preserved without any additional processing and is thus unaffected by --cid-base:

ipfs ls (only the original path displayed when multiple arguments are present, not the actual output) 
ipfs pin update

In addition all commands under bitswap, object, and dag do not, by default, auto-upgrade CIDv0 to CIDv1 as these commands are considered low-level and the conversion could cause problems (for example a dag will not survive a ipfs object get' / 'ipfs object put round trip. This can be changes by using --output-cidv1=true.

Todo:

  • Remove cidenc.Interface for right now it doesn't serve a useful purpose and it may make more sense to define it to only have an Encode method (and no Recode method) so the existing cid.Encoder also implements the interface.
  • Abandon the apicid concept all together as that will be a holdover from the client side implementation and move apicid.Cid to cidenc.Cid.
  • Rebase to clean up commits and avoid depending on locally gx published versions of go-cidutil.
  • Finalize --output-cidv1|--force-multibase-cids|--force-cid-base option name

Depends on ipfs/go-cidutil#10.

Closes #5233. Closes #5234. Closes #5349.

@kevina kevina requested a review from Kubuxu as a code owner November 22, 2018 09:18
@ghost ghost assigned kevina Nov 22, 2018
@ghost ghost added the status/in-progress In progress label Nov 22, 2018
@kevina kevina force-pushed the kevina/multibase4 branch 2 times, most recently from ba61e50 to f124585 Compare November 22, 2018 20:39
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

I think there is a better way to integrate this using CoreAPI:

  • Pick what to use for global options in CoreAPI - CoreApi.Offline #5654 (to be consistent)
  • Define new CID interface:
    • Only for use in user-facing apis, internally we still want to use the string-backed version because performance
    • Instead of apicid.Hash we create implementation of this interface which keeps track of encoding
    • Calling .String on the encoding-tracking impl will get us the right base without having to remember to use cidenc everywhere
  • We switch all coreapi methods to the new cid interface. This should be more contained as most of coreapi operates on path interface, which centralizes the change and makes it harder to miss places that care about encoding.
  • Then we implement global coreapi option which allows user to specify encoding returned by default.
  • If a coreapi method accepts path we use encoding from the given path instead of one defined by the global option
    • Or provide 2 global options, one allowing to override path encoding

@Stebalien @kevina Does that make sense?

@kevina
Copy link
Contributor Author

kevina commented Nov 26, 2018

@magik6k I think that perhaps that is something we can do later. As many of the command listed above have not been converted to the CoreAPI I don't see how it will help much for now. Also, I attempted to do something like you proposed (strong multibase encoding with CID) and received considerable push back from @Stebalien (see ipfs/go-cid#64 and the discussion in #5349). Although in all fairness I was proposing we use the interface type everywhere, if we limit the new CID type to the CoreAPI interface it may become more acceptable.

It sound to be like if we go with your proposal we will effectively be setting the base server side. Correct?

Above else, keeping this P.R. up to date has been a time sink. I would like to get something in as this is a component of the P0 base32 work and overdue by several months. My goal now is to simplify things as much as possible so this can get in quickly. We can work on a better interface later.

@eingenito
Copy link
Contributor

@kevina what's the next steps for this PR? @magik6k do you have comments based on @kevina's response last comment? Do you think it's reasonable to merge based on the current implementation? Or do you want to kick it upstairs to @Stebalien to see what he thinks? If you guys want you might consider making time to chat amongst the three of you - Steven has a Calendly (which he's soon going to regret) available here: https://calendly.com/stebalien/30min/11-28-2018?back=1 so you can make time for a 15 minute sync conversation if that's useful.

@kevina
Copy link
Contributor Author

kevina commented Nov 29, 2018

@eingenito this p.r. is a change in direction from what me and @Stebalien agree on, the other p.r. #5777 is closer to the solution we discussed.

See the comments at the end of #5349 for more context.

I would like to get this p.r. pretty much as is and then if possible refine it later. I specially designed it to be an non-invasive as possible so that we will not be locked into a particular implementation.

We do need to agree on is the choice of flags --cid-base and --output-cidv1 and the fact that we want these command to effect the JSON output, however.

Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Given the time constraints I think this is a decent solution.

We may also want:

  • ipfs dag put - there is no way to specify encoding currently
  • ipfs dag get - might be quite tricky, maybe as a separate PR

core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/pin.go Show resolved Hide resolved
filestore/util.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I'm actually quite impressed at how non-invasive you've been able to make this. This approach looks like it'll work quite well (baring unforeseen circumstances...).

core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

@magik6k

dag get
dag get

See #5349 (comment) and #5349 (comment)

I think there is a better way to integrate this using CoreAPI:

I'd rather make the CoreAPI as clean as possible for libraries. That is, a go application using the CoreAPI as a library should also be able to use a local IPLD DAGService etc. without having to use multiple CID types.


@kevina is there any thing else that needs commenting on for me to unblock this?

@kevina
Copy link
Contributor Author

kevina commented Dec 7, 2018

@Stebalien see #5349 (comment).

@magik6k
Copy link
Member

magik6k commented Dec 7, 2018

I'd rather make the CoreAPI as clean as possible for libraries. That is, a go application using the CoreAPI as a library should also be able to use a local IPLD DAGService etc. without having to use multiple CID types.

If we only apply this to exposed string hashes then yeah, this is fine. I don't think there is anything in CoreAPI that returns those, so 👍.

@kevina
Copy link
Contributor Author

kevina commented Dec 12, 2018

This is almost ready to go. I did some additional simplifications and updated the top-level description. Please reread it.

I will rebase to clean up my commits once near the end to let people who already started to review this a change to see what's new.

@kevina kevina changed the title WIP: Add global option to specify the multibase encoding (server side) Add global option to specify the multibase encoding (server side) Dec 12, 2018
@kevina kevina added the need/review Needs a review label Dec 14, 2018
@kevina
Copy link
Contributor Author

kevina commented Jan 9, 2019

@Stebalien the test failures on Travis CI seam unrelated to my changes, and the shareness tests are passing on Cicle CI and Jenkins so we should be good to go test wise.

@kevina
Copy link
Contributor Author

kevina commented Jan 9, 2019

For the record, one of the reasons I objected to such a long name is in the command line help text, which is now (cut off at 80 cols):

USAGE
  ipfs - Global p2p merkle-dag filesystem.

SYNOPSIS
  ipfs [--config=<config> | -c] [--debug=<debug> | -D] [--help=<help>] [-h=<h>] 

OPTIONS

  -c,                      --config   string - Path to the configuration file to
  -D,                      --debug    bool   - Operate in debug mode.
  --help                              bool   - Show the full command help text.
  -h                                  bool   - Show a short version of the comma
  -L,                      --local    bool   - Run the command locally, instead 
  --offline                           bool   - Run the command offline.
  --api                               string - Use a specific API instance (defa
  --cid-base                          string - Multibase encoding used for versi
  --upgrade-cidv0-in-output           bool   - Upgrade version 0 to version 1 CI
  --enc,                   --encoding string - The encoding type the output shou
  --stream-channels                   bool   - Stream channel output.
  --timeout                           string - Set a global timeout on the comma

SUBCOMMANDS
...

Not a super big deal and something that we can perhaps fix later.

Also, for the record, my original objection to just --upgrade-cidv0 is that we might want to automatically upgrade CIDv0 pins to CIDv1, but now that I think of it, this is something that is best left to a config option as some sort of migration should be used to convert existing pins when this option is set.

But again, I don't think is is a big deal, especially since it won't be used much, and am more interested in getting this in, then anything else, so I am happy with --upgrade-cidv0-in-output.

@magik6k
Copy link
Member

magik6k commented Jan 10, 2019

--upgrade-cidv0-in-output is a bit long, but since the alternatives weren't much shorter, so I'm ok with it.

Travis issue is #5845. Jenkins is another random fail. Also, needs a rebase.

@alanshaw
Copy link
Member

alanshaw commented Jan 14, 2019

I've not got strong feels on this one --upgrade-cidv0-in-output is long but descriptive and as previously observed is unlikely to be used often. 👍

@kevina
Copy link
Contributor Author

kevina commented Jan 14, 2019

@Stebalien let me know when you want to get this in and I will rebase just before.

@Stebalien
Copy link
Member

Stebalien commented Jan 17, 2019

@magik6k can you give this a ✔️?

Nvm. It probably doesn't need another review.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Mostly nits except the panic issue in CidEncoderFromPath. I'm find punting on the nits but they should be easy to fix.

I'll merge as soon as they're fixed and a rebase is done so rebase at will.

core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
// GetLowLevelCidEncoder is like GetCidEncoder but meant to be used by
// lower level commands. It differs from GetCidEncoder in that CIDv0
// and not, by default, auto-upgraded to CIDv1.
func GetLowLevelCidEncoder(req *cmds.Request) (cidenc.Encoder, error) {
Copy link
Member

Choose a reason for hiding this comment

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

These functions should be named by what they do, not where they're used. GetLowLevelCidEncoder doesn't tell me how this is different from GetCidEncoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hate naming, is it okay if we punt on this?

core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/cmdenv/cidbase.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
core/commands/pin.go Outdated Show resolved Hide resolved
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
This does it on ther server side for most commands.

This also adds a global --output-cidv1 option.

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
ParsePath does not preserve the multibase.

License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT
Signed-off-by: Kevin Atkinson <[email protected]>
@kevina
Copy link
Contributor Author

kevina commented Jan 17, 2019

@Stebalien this should be good to go.

Not sure why t0080-repo.sh is failing on CircleCI, it works locally and with Jenkins.

@Stebalien Stebalien force-pushed the kevina/multibase4 branch 2 times, most recently from 28f68de to f96bd5f Compare January 21, 2019 17:30
Primarily, get rid of extractCidString and cidVer. Neither of these functions
did sane things when a path when a path didn't actually include a CID. For
example, "boo" would yield a base32 encoder.

Also:

* Avoid "optional" errors.
* Make it a pure function of the input path.
* Extract the multibase from *any* type of path of the form
  /namespace/cid-like-thing/... This is a DWIM function.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
Copy link
Member

(Everything passed, just not all at once. Jenkins is a not having a good day.)

@Stebalien Stebalien merged commit 0fd7374 into master Jan 21, 2019
@ghost ghost removed the status/in-progress In progress label Jan 21, 2019
@Stebalien
Copy link
Member

🎆 🚀 🎆

@hacdias hacdias deleted the kevina/multibase4 branch May 9, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review topic/cidv1b32 Topic cidv1b32
Projects
None yet
7 participants