Skip to content

feat: implement IPredecessorFindable interface for Repository class#357

Open
wangxiaoxuan273 wants to merge 2 commits intooras-project:mainfrom
wangxiaoxuan273:predecessor-implementation
Open

feat: implement IPredecessorFindable interface for Repository class#357
wangxiaoxuan273 wants to merge 2 commits intooras-project:mainfrom
wangxiaoxuan273:predecessor-implementation

Conversation

@wangxiaoxuan273
Copy link
Contributor

@wangxiaoxuan273 wangxiaoxuan273 commented Mar 5, 2026

What this PR does / why we need it

This change makes Repository class implement the IPredecessorFindable interface, and enables the class to use ExtendedCopy.

Which issue(s) this PR resolves / fixes

Resolves / Fixes #334

Please check the following list

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
  • Does this change require a documentation update?
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have an appropriate license header?

Copilot AI review requested due to automatic review settings March 5, 2026 04:11
@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.78%. Comparing base (5953fb1) to head (cf6caec).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   91.76%   91.78%   +0.02%     
==========================================
  Files          64       64              
  Lines        2755     2764       +9     
  Branches      364      365       +1     
==========================================
+ Hits         2528     2537       +9     
  Misses        138      138              
  Partials       89       89              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements the IPredecessorFindable interface (via IReadOnlyGraphStorage) on the concrete Repository class, enabling it to be used with ExtendedCopyGraphAsync for extended copy operations that traverse the artifact graph through referrers.

Changes:

  • Repository class now declares IReadOnlyGraphStorage in addition to IRepository on its class declaration.
  • A new public GetPredecessorsAsync method is added to Repository, delegating to the existing FetchReferrersAsync (which handles both the Referrers API and tag schema fallback).
  • Two new unit tests cover the happy path (referrers returned) and empty-referrers cases for GetPredecessorsAsync.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/OrasProject.Oras/Registry/Remote/Repository.cs Adds IReadOnlyGraphStorage to Repository's interface list and implements GetPredecessorsAsync by delegating to FetchReferrersAsync.
tests/OrasProject.Oras.Tests/Registry/Remote/RepositoryTest.cs Adds two tests for GetPredecessorsAsync: one with referrers, one with an empty referrers list.

You can also share your feedback on Copilot code review. Take the survey.

- Add <remarks> XML doc to Repository class explaining that
  IRepository does not extend IReadOnlyGraphStorage and callers
  must cast to access graph-related APIs
- Use interface types (IPredecessorFindable/IReadOnlyGraphStorage)
  instead of var for repo in tests per codebase conventions
- Add test for referrers API error path (ResponseException)
- Add test for tag schema fallback path when referrers API is
  not supported

Signed-off-by: Xiaoxuan Wang <[email protected]>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


You can also share your feedback on Copilot code review. Take the survey.

@akashsinghal
Copy link
Collaborator

Caution

This review was generated by Copilot using Claude Opus 4.6, mimicking the review style of shizhMSFT. This is not the real shizhMSFT.

Review of PR #357 — feat: implement IPredecessorFindable interface for Repository class

Implementation is correct and clean — delegates to FetchReferrersAsync, properly materializes the IAsyncEnumerable with ConfigureAwait(false), and returns the list. Test coverage is thorough (happy path, empty, error, tag-schema fallback). Prior review feedback has been addressed (interface-typed test variables, additional test cases). SGTM with comments below.


1. Should IRepository extend IPredecessorFindable?

In oras-go, ReadOnlyGraphTarget composes ReadOnlyGraphStorage + Resolver, and Repository implements it at the interface level. The .NET equivalent would be having IRepository extend IPredecessorFindable (or IReadOnlyGraphStorage). The current approach puts predecessor support only on the concrete Repository class, forcing callers to downcast:

IRepository repo = registry.GetRepositoryAsync("myrepo");
// Cannot call repo.GetPredecessorsAsync() — must cast:
var graphRepo = (IReadOnlyGraphStorage)repo;

This breaks interface-based programming and makes DI harder. The XML <remarks> documenting the cast requirement is a symptom of the interface gap, not a solution.

Consider adding IPredecessorFindable to IRepository (or at least IReadOnlyGraphStorage). If that's a breaking change for existing IRepository implementations, it should still be tracked as a follow-up issue. Will it be better to open an issue for this?

2. nit: Redundant count assertion

In Repository_GetPredecessorsAsync_ShouldReturnReferrers:

Assert.Equal(expectedReferrersList.Count, predecessorList.Count);
Assert.Equivalent(expectedReferrersList, predecessorList);

Assert.Equivalent already validates collection equivalence including count. The Assert.Equal(count) check is redundant. Consider removing it.

3. nit: new CancellationToken() vs CancellationToken.None

All 4 tests use var cancellationToken = new CancellationToken();. Prefer CancellationToken.None or just pass default — it communicates intent ("I don't need cancellation") more clearly and avoids an unnecessary allocation. Same for other tests in the file (pre-existing pattern, so not blocking).

4. nit: Test method name split across lines

public async Task
    Repository_GetPredecessorsAsync_FallbackToTagSchema()

This is harder to scan than a single line. Consider keeping method declarations on one line, even if it exceeds the 100-column soft target — the 120-column hard limit accommodates this:

public async Task Repository_GetPredecessorsAsync_FallbackToTagSchema()

@akashsinghal
Copy link
Collaborator

akashsinghal commented Mar 5, 2026

This is a general question: I'm curious, how do we approach adding new support in this SDK? Is the assumption that the .NET SDK mirrors the APIs AND implementation approaches of oras-go? Or do we intend only for the API surface to be the same? Or is equivalency not a goal at all?

The reason I ask is that there things that make sense from a go perspective that don't translate well to .NET. For example, go is by default duck-typed while C# has explicit binding to interface types from concrete classes. Also Dependency Injection is a big part of consuming .NET SDKs and this doesn't exist really in golang (natively).

I am new to this project so I am genuinely curious what the convention is? Any pointers would be great! thank you.

@wangxiaoxuan273
Copy link
Contributor Author

This is a general question: I'm curious, how do we approach adding new support in this SDK? Is the assumption that the .NET SDK mirrors the APIs AND implementation approaches of oras-go? Or do we intend only for the API surface to be the same? Or is equivalency not a goal at all?

The reason I ask is that there things that make sense from a go perspective that don't translate well to .NET. For example, go is by default duck-typed while C# has explicit binding to interface types from concrete classes. Also Dependency Injection is a big part of consuming .NET SDKs and this doesn't exist really in golang (natively).

I am new to this project so I am genuinely curious what the convention is? Any pointers would be great! thank you.

This is a dotnet SDK after all, so I think goal should be developing a working SDK for dotnet end users, regardless of the golang SDK implementation. We only take Golang SDK for functional reference.

The issue is that I am not very experienced in dotnet development and this project has few experienced dotnet contributors.

@akashsinghal
Copy link
Collaborator

This is a general question: I'm curious, how do we approach adding new support in this SDK? Is the assumption that the .NET SDK mirrors the APIs AND implementation approaches of oras-go? Or do we intend only for the API surface to be the same? Or is equivalency not a goal at all?
The reason I ask is that there things that make sense from a go perspective that don't translate well to .NET. For example, go is by default duck-typed while C# has explicit binding to interface types from concrete classes. Also Dependency Injection is a big part of consuming .NET SDKs and this doesn't exist really in golang (natively).
I am new to this project so I am genuinely curious what the convention is? Any pointers would be great! thank you.

This is a dotnet SDK after all, so I think goal should be developing a working SDK for dotnet end users, regardless of the golang SDK implementation. We only take Golang SDK for functional reference.

The issue is that I am not very experienced in dotnet development and this project has few experienced dotnet contributors.

Thanks for clarifying. I recognize that dotnet has a different language pattern and standard than golang. I'll keep that in mind and help out with reviews as I can, as I have experience with dotnet. I appreciate your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Repository class should implement IPredecessorFindable interface

3 participants