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

Document PubSub changes from go-ipfs v0.11 #1007

Closed
TheDiscordian opened this issue Jan 26, 2022 · 15 comments
Closed

Document PubSub changes from go-ipfs v0.11 #1007

TheDiscordian opened this issue Jan 26, 2022 · 15 comments
Assignees
Labels
dif/easy Someone with a little familiarity can pick up effort/days Estimated to take multiple days, but less than a week good first issue Good issue for new contributors kind/bug A bug in existing code (including security flaws) need/maintainers-input Needs input from the current maintainer(s) P1 High: Likely tackled by core team if no one steps up

Comments

@TheDiscordian
Copy link
Member

TheDiscordian commented Jan 26, 2022

This describes the issue very well: ipfs/kubo#8612

Caused by: ipfs/kubo#8343

Relevant areas: https://docs.ipfs.io/reference/http/api/#api-v0-pubsub-pub && https://docs.ipfs.io/reference/http/api/#api-v0-pubsub-sub (Edit: This actually doesn't effect sub, details below!)

Basically, we used to accept regular strings for pubsub topics over the HTTP API, now we require Base64URL encoding prefixed with a 'u' (Edit: As outlined below, this isn't entirely true, Base64URL is the default/recommended encoding, but technically any multibase should work https://github.com/multiformats/multibase, which is where the u comes from). Noting this down in an issue so I don't forget, I spun my wheels on this for a while not realizing this change happened, this is probably a good first issue though.

Also not sure how other APIs have been effected. I believe the CLI handles this for you for example.

@TheDiscordian TheDiscordian added the need/triage Needs initial labeling and prioritization label Jan 26, 2022
@Annamarie2019
Copy link
Contributor

We made a change and it's causing bugs. We used to accept regular strings for pubsub topics over the HTTP API, now we require Base64URL encoding prefixed with a 'u'.
These docs need to be fixed: https://docs.ipfs.io/reference/http/api/#api-v0-pubsub-pub && https://docs.ipfs.io/reference/http/api/#api-v0-pubsub-sub
Needs analysis as to whether other APIs are affected.

@Annamarie2019 Annamarie2019 added dif/easy Someone with a little familiarity can pick up effort/days Estimated to take multiple days, but less than a week good first issue Good issue for new contributors kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up and removed need/triage Needs initial labeling and prioritization labels Mar 1, 2022
@TMoMoreau
Copy link
Contributor

Fix for the areas specified by @TheDiscordian at PR #1046

@Annamarie2019
Copy link
Contributor

@TMoMoreau or @TheDiscordian Sorry, I'm new to Base64URL encoding and can't find any discussion of it in respect to a prefix. (46 seems to mean u, but that doesn't help me any). I don't know how to prefix the u. Is there a . or an _ ? Is this doc correct (I bolded what I changed):

Arguments

  • arg [string]: Topic to publish to. Required: yes. Requires Base64URL encoding, prefixed with u.

Request Body

Argument data is of file type. This endpoint expects one or several files (depending on the command) in the body of the request as umultipart/form-data.

As for affecting other APIs, publish is the only one that writes and has a Request Body, so it seemed to me that that's the only one to change. Is that correct thinking? (To read existing stuff, you'd have to call it by whatever it's named.)

@TheDiscordian
Copy link
Member Author

TheDiscordian commented Mar 8, 2022

@Annamarie2019 The u comes from multibase encoding: https://github.com/multiformats/multibase. So technically prefixes other than u are possible for other bases. I tested out your theory that this only effects publish and not subscribe, you're totally correct there.

Thanks so much for investigating!

Edit: Also sorry for not updating this issue when I realised other prefixes are possible! Totally slipped my mind.

@Annamarie2019
Copy link
Contributor

Annamarie2019 commented Mar 8, 2022

@TheDiscordian @TMoMoreau

For reference, this is what the article says now:


/api/v0/pubsub/pub

Publish data to a given pubsub topic.

Arguments
arg [string]: Topic to publish to. Required: yes.

Request Body
Argument data is of file type. This endpoint expects one or several files (depending on the command) in the body of the request as 'multipart/form-data'.


I'll display my ignorance here, in case it helps the cause, but I don't understand:

  • "of file type"... Do we mean, "Argument data is the endpoint, which expects one or more files"...or are we asking for a file type (.txt, .mp4...)?
  • "depending on the command"....I thought we were defining the command: pub.... Does it mean "sub-command" or "argument"? (such as " ipfs multibase encode - Encode data into multibase string " ...from entering ipfs multibase u --help in cli)
  • "as" .... meaning "such as" (introducing an example) or "in the format of"
  • the original says "as multipart/form-data Multibase format is <base-encoding-character><base-encoded-data>".....does multipart/form-data need some explanation? (See What does multipart/form-data mean? and HTML | enctype Attribute ) Would the accepted format be umultipart/form-data or possibly /u/multiformat/form-data or other? How do you format multiple files...comma delimited? space? How do you format multiple files?
    Other than u, are other bases possible? under what conditions?

@Annamarie2019 Annamarie2019 added the help wanted Seeking public contribution on this issue label Mar 8, 2022
@johnnymatthews johnnymatthews added need/maintainers-input Needs input from the current maintainer(s) and removed help wanted Seeking public contribution on this issue labels Mar 8, 2022
@TheDiscordian
Copy link
Member Author

TheDiscordian commented Mar 8, 2022

data/body is out of scope of this issue (hasn't been changed). arg is what this issue is about, and the formats other than u (Base64URL) are defined here: https://github.com/multiformats/multibase, as I outlined above.

Sorry, I'm genuinely not understanding the disconnect here. @TMoMoreau should be able to update his PR with this new info though.

Edit: BTW some of the confusion you've outlined really might be worth new issues to help with clarity.

@TMoMoreau
Copy link
Contributor

Updated PR to document the changes that this issue specified. #1065

@lidel
Copy link
Member

lidel commented Mar 8, 2022

Related: ipfs/go-ipfs-cmds#224

@Annamarie2019
Copy link
Contributor

Thanks, @TheDiscordian I read so much stuff, that i forgot where I started: "we used to accept regular strings for pubsub topics...now we require....." (Duh!) So how's this for the update:

Arguments
arg [string]: Topic to publish to. Required: yes. Requires multibase encoding. The default and recommended encoding is u for base64url, but you can use any base encoding character. See the Multibase Table at multibase.

@Annamarie2019
Copy link
Contributor

Annamarie2019 commented Mar 9, 2022

@TMoMoreau Thanks. Your explanation in #1065 is great, but if we have it up by the argument, maybe after the curl example, all we need to say is, something like:

Arguments
arg [string]: Topic to publish to. Required: yes. Requires multibase encoding. The default and recommended encoding is u for base64url, but you can use any base encoding character. See the Multibase Table at multibase.

Request Body...

curl -X POST -F file=@myfile "http://127.0.0.1:5001/api/v0/pubsub/pub?arg=u<topic>"
Note the u prefix before the topic name, satisfying the argument requirement to be base-encoded.

cc: @TheDiscordian

@Annamarie2019 Annamarie2019 self-assigned this Mar 10, 2022
@Annamarie2019 Annamarie2019 added the status/blocked Unable to be worked further until needs are met label Mar 10, 2022
@TheDiscordian TheDiscordian removed the status/blocked Unable to be worked further until needs are met label Mar 11, 2022
@Annamarie2019
Copy link
Contributor

Annamarie2019 commented Mar 14, 2022

@TMoMoreau @TheDiscordian Should we add the u prefix before the topics and link to this explanation for https://docs.ipfs.io/reference/cli/#ipfs-pubsub in the subcommands (maybe tick marks around those commands as well)?

@TheDiscordian
Copy link
Member Author

@lidel First, reading the source, both pub and sub expect multibase, yes? If so, is this bandaid okay (until maybe a new type can be brewed up):

https://github.com/ipfs/go-ipfs/blob/a61c53f87fc6b5341c8d0a0b088cb16dd59f8cc8/core/commands/pubsub.go#L173 "Topic to publish to." -> "Topic to publish to (multibase encoded)."
https://github.com/ipfs/go-ipfs/blob/a61c53f87fc6b5341c8d0a0b088cb16dd59f8cc8/core/commands/pubsub.go#L78 "Name of topic to subscribe to." -> "Name of topic to subscribe to (multibase encoded)."

I specifically want to know if sub requires the multibase encoding because IIRC when I tested it out, it didn't need it, but it's possible (likely even) that I didn't test thoroughly enough.

If so @TMoMoreau can make a PR with those changes. And hopefully put this issue to bed until a better issue spawns.

CC @johnnymatthews

@lidel
Copy link
Member

lidel commented Apr 5, 2022

iirc all ipfs pubsub * commands got fixed to use multibase when topic is passed as arg (ipfs/kubo#8183 (review)) so this impacts both pub and sub.

The proposed bandaid is fine until ipfs/go-ipfs-cmds#224 is addressed 👍

There is a small caveat tho: ipfs command line client will do the multibase conversion for you automatically. So on the CLI you use regular strings, and the conversion happens behind the scenes before the wire format is sent over HTTP RPC.

Due to this, we need to clarify that in a way that looks acceptable on both HTTP RPC API reference page, and when user reads ipfs pubsub pub --help (unfortunately, the same text is displayed in both places).

Pubsub commands are marked as experimental, so we don't need to stress too much about this, does not have to be pretty.

Maybe (multibase encoded when sent over HTTP RPC) ?

TMoMoreau added a commit to TMoMoreau/go-ipfs that referenced this issue May 3, 2022
Adds clarification for pubsub multibase encoding over HTTP RPC for issue ipfs/ipfs-docs#1007
BigLep pushed a commit to ipfs/kubo that referenced this issue May 6, 2022
* pubsub multibase encoding

Adds clarification for pubsub multibase encoding over HTTP RPC for issue ipfs/ipfs-docs#1007

* Grammatical change

* Moved period
@BigLep
Copy link
Contributor

BigLep commented May 6, 2022

Closing this since ipfs/kubo#8933 was merged

@BigLep BigLep closed this as completed May 6, 2022
@lidel
Copy link
Member

lidel commented May 16, 2022

Remaining work documented in #1146

guseggert pushed a commit to ipfs/kubo that referenced this issue Jun 8, 2022
* pubsub multibase encoding

Adds clarification for pubsub multibase encoding over HTTP RPC for issue ipfs/ipfs-docs#1007

* Grammatical change

* Moved period
guseggert pushed a commit to ipfs/kubo that referenced this issue Jun 8, 2022
* pubsub multibase encoding

Adds clarification for pubsub multibase encoding over HTTP RPC for issue ipfs/ipfs-docs#1007

* Grammatical change

* Moved period

(cherry picked from commit 9a84a4f)
guseggert pushed a commit to ipfs/kubo that referenced this issue Jun 8, 2022
* pubsub multibase encoding

Adds clarification for pubsub multibase encoding over HTTP RPC for issue ipfs/ipfs-docs#1007

* Grammatical change

* Moved period

(cherry picked from commit 9a84a4f)
guseggert pushed a commit to ipfs/kubo that referenced this issue Jun 8, 2022
* pubsub multibase encoding

Adds clarification for pubsub multibase encoding over HTTP RPC for issue ipfs/ipfs-docs#1007

* Grammatical change

* Moved period

(cherry picked from commit 9a84a4f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dif/easy Someone with a little familiarity can pick up effort/days Estimated to take multiple days, but less than a week good first issue Good issue for new contributors kind/bug A bug in existing code (including security flaws) need/maintainers-input Needs input from the current maintainer(s) P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

No branches or pull requests

6 participants