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(gateway): improve GO API interface, remove Writable API #145

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 31, 2023

Closes #61

This PR does the following:

  1. Simplifies NodeAPI (renamed API), such that we only require the bare minimum functions we need on the gateway. These have each the minimum required amount of functions required. This will allow other developers to use our gateway code much more easily.
  2. Consolidates the offlineAPI as a single function on API (IsCached).
  3. Removes the Writable Gateway. The code for the Writable Gateway is moved to Kubo codebase and deprecated Deprecate Gateway.Writable in Kubo 0.19 kubo#9622. See refactor: new go-libipfs/gateway API, deprecate Gateway.Writable kubo#9616.

Please note: gateway sharness tests are expected to fail since they are run against Kubo's master version. The PR in Kubo with this changes can be viewed at: ipfs/kubo#9616

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #145 (c7cc8b8) into main (7346505) will increase coverage by 0.32%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     ipfs/in-web-browsers#145      +/-   ##
==========================================
+ Coverage   17.95%   18.28%   +0.32%     
==========================================
  Files          94       94              
  Lines       10368    10181     -187     
==========================================
  Hits         1862     1862              
+ Misses       8245     8058     -187     
  Partials      261      261              
Impacted Files Coverage Δ
gateway/gateway.go 0.00% <ø> (ø)
gateway/handler.go 5.32% <0.00%> (+1.27%) ⬆️
gateway/handler_block.go 0.00% <0.00%> (ø)
gateway/handler_car.go 0.00% <0.00%> (ø)
gateway/handler_codec.go 0.00% <0.00%> (ø)
gateway/handler_ipns_record.go 0.00% <0.00%> (ø)
gateway/handler_tar.go 0.00% <0.00%> (ø)
gateway/handler_unixfs.go 0.00% <0.00%> (ø)
gateway/handler_unixfs__redirects.go 0.00% <0.00%> (ø)
gateway/handler_unixfs_dir.go 0.00% <0.00%> (ø)
... and 1 more

@hacdias hacdias changed the title refactor: decouple read-write API code refactor(gateway): decouple read, write, and offline Node APIs Jan 31, 2023
@hacdias hacdias force-pushed the refactor/decouple-read-write-gateway branch from ac1614c to 43deb6b Compare January 31, 2023 12:12
@hacdias hacdias self-assigned this Jan 31, 2023
@hacdias hacdias marked this pull request as ready for review January 31, 2023 12:52
@hacdias hacdias requested a review from lidel as a code owner January 31, 2023 12:52
@hacdias hacdias marked this pull request as draft January 31, 2023 15:58
@hacdias
Copy link
Member Author

hacdias commented Jan 31, 2023

I converted this to draft because I will be doing to more things: removing the Writable gateway and updating the interface to make it more simple.

@hacdias hacdias changed the title refactor(gateway): decouple read, write, and offline Node APIs feat(gateway): improve API interface, remove Writable API Jan 31, 2023
@hacdias hacdias marked this pull request as ready for review January 31, 2023 17:13
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you for identifying these @hacdias.

I've dropped some ideas to simplify it further + avoid confusing names + follow up for cleaning up writable mode in Kubo.

gateway/gateway.go Outdated Show resolved Hide resolved
gateway/gateway.go Outdated Show resolved Hide resolved
gateway/gateway.go Outdated Show resolved Hide resolved
gateway/handler_car.go Outdated Show resolved Hide resolved
gateway/handler_codec.go Outdated Show resolved Hide resolved
gateway/gateway.go Outdated Show resolved Hide resolved
gateway/gateway.go Outdated Show resolved Hide resolved
gateway/gateway.go Outdated Show resolved Hide resolved
gateway/handler.go Show resolved Hide resolved
@lidel lidel mentioned this pull request Feb 1, 2023
2 tasks
@hacdias hacdias force-pushed the refactor/decouple-read-write-gateway branch from 1e462b6 to 7c44868 Compare February 1, 2023 09:05
@hacdias hacdias requested a review from lidel February 1, 2023 09:25
Comment on lines 107 to 114
// Optimization: use Unixfs.Ls without resolving children, but using the
// cumulative DAG size as the file size. This allows for a fast listing
// while keeping a good enough Size field.
results, err := i.api.LsUnixFsDir(ctx,
resolvedPath,
options.Unixfs.ResolveChildren(false),
options.Unixfs.UseCumulativeSize(true),
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this optimization to Kubo's codebase. Each gateway implementation should use whatever they want, some might be faster, some slower. It's the implementers choice. This also allowed me to simplify the API signature to remove the options.

- LsUnixFsDir(context.Context, path.Path, ...options.UnixfsLsOption) (<-chan iface.DirEntry, error)
+ LsUnixFsDir(context.Context, path.Resolved) (<-chan iface.DirEntry, error)

Comment on lines 64 to 71
idx, err := i.api.GetUnixFsNode(ctx, idxPath)
idxResolvedPath, err := i.api.ResolvePath(ctx, idxPath)
switch err.(type) {
case nil:
idx, err := i.api.GetUnixFsNode(ctx, idxResolvedPath)
if err != nil {
internalWebError(w, err)
return
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the signature of the getter for UnixFS files:

- GetUnixFsNode(context.Context, path.Path) (files.Node, error)
+ GetUnixFsNode(context.Context, path.Resolved) (files.Node, error)

We already have ResolvePath at our disposal. Let's simplify. The change in this code is to reflect that.

@lidel lidel changed the title feat(gateway): improve API interface, remove Writable API feat(gateway): improve GO API interface, remove Writable API Feb 2, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @hacdias, moving Gateway.Writable to ipfs/kubo#9616 is 👍
Merging to unblock other PRs based on this.

// Unixfs returns an implementation of Unixfs API
Unixfs() coreiface.UnixfsAPI
// API defines the minimal set of API services required for a gateway handler.
type API interface {
Copy link
Member

@lidel lidel Feb 2, 2023

Choose a reason for hiding this comment

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

nit: something like IPFSBackend would be more descriptive, but can be changed later, won't block on this one thing (this interface will most likely get revamp in near future anyway)

@lidel lidel merged commit 6399b73 into main Feb 2, 2023
@lidel lidel deleted the refactor/decouple-read-write-gateway branch February 2, 2023 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

gateway: post-extraction GO API cleanup
2 participants