Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor(providerindex): accept multiple legacy options #86

Merged
merged 13 commits into from
Feb 3, 2025

Conversation

hannahhoward
Copy link
Member

Goals

Get a full roundtrip to work with freeway -- these are all the additional changes required

Implementation

  • update go-capabilities to include fixes from fix(assert): fix casing on other caveats schema types go-capabilities#6 (which had not been updated in a minute, so there are a few changes required)
  • modify the legacy claims finder to work with multiple claim mappers, and be sensitive to target claims. the reason for this is as follows -- I had assumed by fallback mapper could simply run on top of the base legacy mapper and run when the legacy mapper returns no claims. however, in practice, we end up in a situation where we are specifically looking for a Shard CID location claim, and the base legacy mapper returns a Shard CID EQUALS claim. So we need to have that filtered out and then run my fallback mapper anyway. This ends up making much more sense to do at the legacy claims finder level than in the fallback mapper.

For Discussion

Still need to add some new test cases for the legacy claims finders now that it uses multiple mappers & handles filtering

Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 74.50980% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/construct/construct.go 0.00% 4 Missing ⚠️
pkg/service/providerindex/legacy.go 87.09% 3 Missing and 1 partial ⚠️
pkg/aws/service.go 0.00% 3 Missing ⚠️
pkg/internal/testutil/gen.go 80.00% 1 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
pkg/aws/bucketfallbackmapper.go 74.46% <100.00%> (-2.46%) ⬇️
pkg/service/providerindex/providerindex.go 33.33% <100.00%> (-1.93%) ⬇️
pkg/internal/testutil/gen.go 83.82% <80.00%> (-0.31%) ⬇️
pkg/aws/service.go 0.00% <0.00%> (ø)
pkg/construct/construct.go 0.00% <0.00%> (ø)
pkg/service/providerindex/legacy.go 73.97% <87.09%> (+1.09%) ⬆️

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Incredible debugging!

Copy link
Member

@volmedo volmedo left a comment

Choose a reason for hiding this comment

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

that's an interesting corner case! Implementation looks good, I left some minor comments.

}

// LegacyClaimsStore allows finding claims on a legacy store
type LegacyClaimsStore struct {
contentToClaims ContentToClaimsMapper
contentToClaims []ContentToClaimsMapper
Copy link
Member

Choose a reason for hiding this comment

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

np: I'd rename this to mappers or something like that, now that there may be several of them

return nil, nil
}

func (ls LegacyClaimsStore) findOne(ctx context.Context, contentHash multihash.Multihash, targetClaims []multicodec.Code, mapper ContentToClaimsMapper) ([]model.ProviderResult, error) {
Copy link
Member

Choose a reason for hiding this comment

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

why is this function called findOne? maybe it had a different goal before?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's checking one mapper when the function above checks many

Copy link
Member

Choose a reason for hiding this comment

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

got it. The very first thing that came to my mind was "find one result". I guess I'm too used to datastore APIs :)

Comment on lines 71 to 79
for _, mapper := range ls.contentToClaims {
claims, err := ls.findOne(ctx, contentHash, targetClaims, mapper)
if err != nil {
return nil, err
}
if len(claims) > 0 {
return claims, nil
}
}
Copy link
Member

Choose a reason for hiding this comment

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

the results of this logic will depend on the order the mappers are provided in the slice. The results being returned will be the ones given by the first mapper returning any result.

Not saying it's wrong, but it may be worth stating it clearly somewhere (in the comment for NewLegacyClaimsStore, maybe?).

Copy link
Member Author

Choose a reason for hiding this comment

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

yes that's the goal, but it's absolutely worst stating

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a test for correct filtering of claims according to targetClaims?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea

@volmedo volmedo changed the base branch from fix/claim-fallback-cid to main February 2, 2025 02:07
@volmedo
Copy link
Member

volmedo commented Feb 2, 2025

I tidied some things up, updated the branch with the latest main and added tests to confirm handling of different mappers and claim filtering work as expected.

This is RFAL.

@volmedo volmedo marked this pull request as ready for review February 2, 2025 12:44
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Still LGTM

@volmedo volmedo merged commit b43aefa into main Feb 3, 2025
9 checks passed
@volmedo volmedo deleted the refactor/handle-multiple-claim-stacks branch February 3, 2025 11:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants