Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: support block.rm over http api #2514

Merged
merged 10 commits into from
Oct 8, 2019
Merged

feat: support block.rm over http api #2514

merged 10 commits into from
Oct 8, 2019

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Oct 5, 2019

Also, validate that a block is not pinned before removing it.

Might, uh, might want to do that.

@achingbrain achingbrain mentioned this pull request Oct 6, 2019
52 tasks
src/http/api/resources/block.js Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member Author

Needs ipfs/interop#89 for CI to pass.

@achingbrain achingbrain merged commit c9be79e into master Oct 8, 2019
@achingbrain achingbrain deleted the expose-block-rm branch October 8, 2019 09:53
@AuHau
Copy link
Member

AuHau commented Oct 9, 2019

Hmmm this broke few things 😅

First of all the change of taking multiple CIDs into rm() should be also propagated to SPEC into interface-js-ipfs-core, if I am not mistaken. Maybe you are already working on it?

Second of all quiet, some tests were expecting for ipfs.block.rm() to raise some exceptions at some point. For example https://github.com/ipfs/js-ipfs/blob/master/test/core/gc.spec.js#L193 . I will fix this one as I am working on GC related stuff, but you should make sure that other parts of tests are not silently broken because they might not be correctly written (not expecting not to throw like in the GC tests). Not sure if it can be the case somewhere else...

Lastly one general question about programming attitude. When you have bulk operations (like bulk remove of blocks), do you fail-fast or do as much as possible? From the perspective of the backward compatibility of this change, it would make sense to fail-fast and don't catch the exceptions, but not sure what is the general approach about this across IPFS JS-land...

@achingbrain

@achingbrain
Copy link
Member Author

achingbrain commented Oct 9, 2019

Looks like that test should be refactored to enforce that the exception is thrown, if that's valid behaviour e.g:

// Second rm should fail because GC has already removed that block
await rm2.then(
  () => expect.fail('should have thrown when trying to remove already removed block'),
  (err) => expect(err.code).eql(Errors.dbDeleteFailedError().code)
)

I think in general we try to fail fast, but there are a few exceptions. block.rm is one of them as go-IPFS doesn't fail fast in this case and we need to maintain compatibility.

@AuHau
Copy link
Member

AuHau commented Oct 9, 2019

I see, alright make sense.

Sure, I will do that. But at the same time, looking on diff of this PR, it is bit suspicious that you did not change any tests which would depend on block.rm() either throw exceptions or fail at some point, which I would think should be also somewhere else then just at the gc part.

@achingbrain
Copy link
Member Author

achingbrain commented Oct 9, 2019

All the tests in this repo passed, plus most of the block.rm tests are in the interface-ipfs-core repo.

In general we want to add tests there and not here where possible so they get run against ipfs-http-client (and as such, go-IPFS) as well.

Side note: I find this sort of development flow hard to work with, maybe you do too, in which case chime in on #2446

@AuHau
Copy link
Member

AuHau commented Oct 9, 2019

Hmmm well, then I found an inconsistency.

https://github.com/ipfs/js-ipfs/blob/master/test/core/block.spec.js#L56 expects to throw on invalid CID. Which is happening and correctly passing. But with your introduced change it should not throw but return an array:

[
 { error: "Invalid CID" }
] 

Right? So currently validation of CID fails fast, but deletion errors do not...

@AuHau
Copy link
Member

AuHau commented Oct 9, 2019

@AuHau
Copy link
Member

AuHau commented Oct 9, 2019

Also, I assume that there is a plan to go all "async" and hence support generators at some point? Maybe it would be worth to track the points where currently generators are degraded to arrays because of this, so the final refactor switch could be easier?

@achingbrain
Copy link
Member Author

achingbrain commented Oct 9, 2019

So currently validation of CID fails fast, but deletion errors do not...

This is intentional. Sort of. It's what go-IPFS does and we are bound by that contract.

Also maybe https://github.com/ipfs/js-ipfs/blob/master/test/core/pin.js#L208 could be un-skipped?

Yes, though better to rewrite it so it doesn't affect other tests in the suite so they can all be run in isolation as well as together.

I assume that there is a plan to go all "async" and hence support generators at some point?

Yes! It's happening in two stages, the first is #1670 to convert everything internally, then #2509 to expose it to the wider world in a more refined API that reflects what we've learnt over the past few years (and also so we stop having to say "it behaves like this because that's what go does" about everything).

Maybe it would be worth to track the points where currently generators are degraded to arrays

This should really only be happening at the very last moment - e.g. in the functions in src/core/components

achingbrain added a commit that referenced this pull request Oct 13, 2019
We had a few instances in the tests where assertions may be missed
due to functions not throwing where we thought they would so this
PR refactors that code to use `.then(onResult, onReject)` instead.

Follows on from #2514 (comment)
achingbrain added a commit that referenced this pull request Oct 13, 2019
We had a few instances in the tests where assertions may be missed
due to functions not throwing where we thought they would so this
PR refactors that code to use `.then(onResult, onReject)` instead.

Follows on from #2514 (comment)
achingbrain added a commit that referenced this pull request Oct 14, 2019
We had a few instances in the tests where assertions may be missed
due to functions not throwing where we thought they would so this
PR refactors that code to use `.then(onResult, onReject)` instead.

Follows on from #2514 (comment)
achingbrain added a commit that referenced this pull request Oct 15, 2019
* chore: remove try-catch from tests where functions are async

We had a few instances in the tests where assertions may be missed
due to functions not throwing where we thought they would so this
PR refactors that code to use `.then(onResult, onReject)` instead.

Follows on from #2514 (comment)
achingbrain added a commit that referenced this pull request Oct 15, 2019
* chore: remove try-catch from tests where functions are async

We had a few instances in the tests where assertions may be missed
due to functions not throwing where we thought they would so this
PR refactors that code to use `.then(onResult, onReject)` instead.

Follows on from #2514 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants