resolve comments

Signed-off-by: Patrick Pan <[email protected]>
oras-project · Dec 13, 2024 · c30d6b2 · c30d6b2
1 parent 7df36ca
commit c30d6b2
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 74 deletions.
diff --git a/src/OrasProject.Oras/Oci/Descriptor.cs b/src/OrasProject.Oras/Oci/Descriptor.cs
@@ -74,10 +74,10 @@ public static Descriptor Create(Span<byte> data, string mediaType)
 
     internal static bool IsEmptyOrInvalid(Descriptor? descriptor)
     {
-        return descriptor == null || descriptor.Size == 0 || string.IsNullOrEmpty(descriptor.Digest) || string.IsNullOrEmpty(descriptor.MediaType);
+        return descriptor == null || string.IsNullOrEmpty(descriptor.Digest) || string.IsNullOrEmpty(descriptor.MediaType);
     }
 
-    internal static Descriptor EmptyDescriptor() => new ()
+    internal static Descriptor ZeroDescriptor() => new ()
     {
         MediaType = "",
         Digest = "",

diff --git a/src/OrasProject.Oras/Registry/Remote/HttpResponseMessageExtensions.cs b/src/OrasProject.Oras/Registry/Remote/HttpResponseMessageExtensions.cs
@@ -111,15 +111,15 @@ internal static void CheckOCISubjectHeader(this HttpResponseMessage response, Re
     {
         if (response.Headers.TryGetValues("OCI-Subject", out var values))
         {
-            // Set it to ReferrerSupported when the response header contains OCI-Subject
-            repository.SetReferrersState(Referrers.ReferrersState.ReferrersSupported);
+            // Set it to Supported when the response header contains OCI-Subject
+            repository.ReferrersState = Referrers.ReferrersState.Supported;
         }
 
         // If the "OCI-Subject" header is NOT set, it means that either the manifest
         // has no subject OR the referrers API is NOT supported by the registry.
         //
         // Since we don't know whether the pushed manifest has a subject or not,
-        // we do not set the ReferrerState to ReferrerNotSupported here.
+        // we do not set the ReferrerState to NotSupported here.
     }
 
     /// <summary>

diff --git a/src/OrasProject.Oras/Registry/Remote/ManifestStore.cs b/src/OrasProject.Oras/Registry/Remote/ManifestStore.cs
@@ -177,7 +177,7 @@ private async Task PushWithIndexingAsync(Descriptor expected, Stream content, Re
         {   
             case MediaType.ImageManifest:
             case MediaType.ImageIndex:
-                if (Repository.ReferrersState == Referrers.ReferrersState.ReferrersSupported)
+                if (Repository.ReferrersState == Referrers.ReferrersState.Supported)
                 { 
                     // Push the manifest straightaway when the registry supports referrers API
                     await DoPushAsync(expected, content, reference, cancellationToken).ConfigureAwait(false);
@@ -190,7 +190,7 @@ private async Task PushWithIndexingAsync(Descriptor expected, Stream content, Re
                     // Push the manifest when ReferrerState is Unknown or NotSupported
                     await DoPushAsync(expected, contentDuplicate, reference, cancellationToken).ConfigureAwait(false);
                 }
-                if (Repository.ReferrersState == Referrers.ReferrersState.ReferrersSupported)
+                if (Repository.ReferrersState == Referrers.ReferrersState.Supported)
                 {
                     // Early exit when the registry supports Referrers API
                     // No need to index referrers list
@@ -228,14 +228,20 @@ private async Task ProcessReferrersAndPushIndex(Descriptor desc, Stream content,
         {
             case MediaType.ImageIndex:
                 var indexManifest = JsonSerializer.Deserialize<Index>(content);
-                if (indexManifest?.Subject == null) return;
+                if (indexManifest?.Subject == null)
+                {
+                    return;
+                }
                 subject = indexManifest.Subject;
                 desc.ArtifactType = indexManifest.ArtifactType;
                 desc.Annotations = indexManifest.Annotations;
                 break;
             case MediaType.ImageManifest:
-                var imageManifest = JsonSerializer.Deserialize<Manifest>(content); 
-                if (imageManifest?.Subject == null) return;
+                var imageManifest = JsonSerializer.Deserialize<Manifest>(content);
+                if (imageManifest?.Subject == null)
+                {
+                    return;
+                }
                 subject = imageManifest.Subject;
                 desc.ArtifactType = string.IsNullOrEmpty(imageManifest.ArtifactType) ? imageManifest.Config.MediaType : imageManifest.ArtifactType;
                 desc.Annotations = imageManifest.Annotations;
@@ -244,7 +250,7 @@ private async Task ProcessReferrersAndPushIndex(Descriptor desc, Stream content,
                 return;
         }
 
-        Repository.SetReferrersState(Referrers.ReferrersState.ReferrersNotSupported);
+        Repository.ReferrersState = Referrers.ReferrersState.NotSupported;
         await UpdateReferrersIndex(subject, new Referrers.ReferrerChange(desc, Referrers.ReferrerOperation.ReferrerAdd), cancellationToken).ConfigureAwait(false);
     }
 
@@ -306,7 +312,7 @@ private async Task UpdateReferrersIndex(Descriptor subject,
     /// <param name="referrersTag"></param>
     /// <param name="cancellationToken"></param>
     /// <returns></returns>
-    internal async Task<(Descriptor, IList<Descriptor>)> PullReferrersIndexList(String referrersTag, CancellationToken cancellationToken = default)
+    internal async Task<(Descriptor?, IList<Descriptor>)> PullReferrersIndexList(String referrersTag, CancellationToken cancellationToken = default)
     {
         try
         {
@@ -320,7 +326,7 @@ private async Task UpdateReferrersIndex(Descriptor subject,
         }
         catch (NotFoundException)
         {
-            return (Descriptor.EmptyDescriptor(), new List<Descriptor>());
+            return (null, new List<Descriptor>());
         }
     }
 

diff --git a/src/OrasProject.Oras/Registry/Remote/Referrers.cs b/src/OrasProject.Oras/Registry/Remote/Referrers.cs
@@ -19,13 +19,13 @@
 
 namespace OrasProject.Oras.Registry.Remote;
 
-public class Referrers
+internal static class Referrers
 {
     internal enum ReferrersState
     {
-        ReferrersUnknown = 0,
-        ReferrersSupported = 1,
-        ReferrersNotSupported = 2
+        Unknown = 0,
+        Supported = 1,
+        NotSupported = 2
     }
 
     internal record ReferrerChange(Descriptor Referrer, ReferrerOperation ReferrerOperation);
@@ -38,8 +38,7 @@ internal enum ReferrerOperation
 
     internal static string BuildReferrersTag(Descriptor descriptor)
     {
-        var validatedDigest = Digest.Validate(descriptor.Digest);
-        return validatedDigest.Substring(0, validatedDigest.IndexOf(':')) + "-" + validatedDigest.Substring(validatedDigest.IndexOf(':') + 1);
+        return Digest.Validate(descriptor.Digest).Replace(':', '-');
     }
 
     /// <summary>

diff --git a/src/OrasProject.Oras/Registry/Remote/Repository.cs b/src/OrasProject.Oras/Registry/Remote/Repository.cs
@@ -47,12 +47,23 @@ public class Repository : IRepository
 
     public RepositoryOptions Options => _opts;
 
-    private int _referrersState = (int) Referrers.ReferrersState.ReferrersUnknown;
+    private int _referrersState = (int) Referrers.ReferrersState.Unknown;
 
+    /// <summary>
+    /// ReferrersState indicates the Referrers API state of the remote repository.
+    /// ReferrersState can be set only once, otherwise it throws ReferrersStateAlreadySetException.
+    /// </summary>
     internal Referrers.ReferrersState ReferrersState
     {
         get => (Referrers.ReferrersState) _referrersState;
-        private set => _referrersState = (int) value;
+        set
+        {
+            var originalReferrersState = (Referrers.ReferrersState) Interlocked.CompareExchange(ref _referrersState, (int)value, (int)Referrers.ReferrersState.Unknown);
+            if (originalReferrersState != Referrers.ReferrersState.Unknown && _referrersState != (int)value)
+            {
+                throw new ReferrersStateAlreadySetException($"current referrers state: {ReferrersState}, latest referrers state: {value}");
+            }
+        }
     }
 
     internal static readonly string[] DefaultManifestMediaTypes =
@@ -93,30 +104,6 @@ public Repository(RepositoryOptions options)
         _opts = options;
     }
 
-    /// <summary>
-    /// SetReferrersState indicates the Referrers API state of the remote repository.
-    ///
-    /// SetReferrersState is valid only when it is called for the first time.
-    /// SetReferrersState returns ReferrersStateAlreadySetException if the
-    /// Referrers API state has been already set.
-    ///   - When the state is set to ReferrersSupported, the Referrers() function will always
-    ///     request the Referrers API. Reference: https://github.com/opencontainers/distribution-spec/blob/v1.1.0/spec.md#listing-referrers
-    ///   - When the state is set to ReferrersNotSupported, the Referrers() function will always
-    ///     request the Referrers Tag. Reference: https://github.com/opencontainers/distribution-spec/blob/v1.1.0/spec.md#referrers-tag-schema
-    ///  - When the state is not set, the Referrers() function will automatically
-    ///     determine which API to use.
-    /// </summary>
-    /// <param name="state"></param>
-    /// <exception cref="ReferrersStateAlreadySetException"></exception>
-    internal void SetReferrersState(Referrers.ReferrersState state)
-    {
-        var originalReferrersState = (Referrers.ReferrersState) Interlocked.CompareExchange(ref _referrersState, (int)state, (int)Referrers.ReferrersState.ReferrersUnknown);
-        if (originalReferrersState != Referrers.ReferrersState.ReferrersUnknown && _referrersState != (int) state)
-        {
-            throw new ReferrersStateAlreadySetException($"current referrers state: {ReferrersState}, latest referrers state: {state}");
-        }
-    }
-
     /// <summary>
     /// FetchAsync fetches the content identified by the descriptor.
     /// </summary>

diff --git a/tests/OrasProject.Oras.Tests/Remote/ManifestStoreTest.cs b/tests/OrasProject.Oras.Tests/Remote/ManifestStoreTest.cs
@@ -100,7 +100,7 @@ public async Task ManifestStore_PullReferrersIndexListNotFound()
         var cancellationToken = new CancellationToken();
         var store = new ManifestStore(repo);
         var (receivedDesc, receivedManifests) = await store.PullReferrersIndexList("test", cancellationToken);
-        Assert.True(Descriptor.IsEmptyOrInvalid(receivedDesc));
+        Assert.Null(receivedDesc);
         Assert.Empty(receivedManifests);
     }
 
@@ -167,14 +167,14 @@ public async Task ManifestStore_PushAsyncWithoutSubject()
         var cancellationToken = new CancellationToken();
         var store = new ManifestStore(repo);
 
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
         await store.PushAsync(expectedManifestDesc, new MemoryStream(expectedManifestBytes), cancellationToken);
         Assert.Equal(expectedManifestBytes, receivedManifest);
 
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
         await store.PushAsync(expectedConfigDesc, new MemoryStream(expectedConfigBytes), cancellationToken);
         Assert.Equal(expectedConfigBytes, receivedManifest);
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
     }
 
 
@@ -254,15 +254,15 @@ public async Task ManifestStore_PushAsyncWithSubjectAndReferrerSupported()
         var store = new ManifestStore(repo);
 
         // first push with image manifest
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
         await store.PushAsync(expectedManifestDesc, new MemoryStream(expectedManifestBytes), cancellationToken);
         Assert.Equal(expectedManifestBytes, receivedManifest);
-        Assert.Equal(Referrers.ReferrersState.ReferrersSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Supported, repo.ReferrersState);
 
         // second push with index manifest
         await store.PushAsync(expectedIndexManifestDesc, new MemoryStream(expectedIndexManifestBytes), cancellationToken);
         Assert.Equal(expectedIndexManifestBytes, receivedManifest);
-        Assert.Equal(Referrers.ReferrersState.ReferrersSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Supported, repo.ReferrersState);
     }
 
 
@@ -377,18 +377,18 @@ public async Task ManifestStore_PushAsyncWithSubjectAndReferrerNotSupported()
         var store = new ManifestStore(repo);
 
         // First push with referrer tag schema
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
         await store.PushAsync(firstExpectedManifestDesc, new MemoryStream(firstExpectedManifestBytes), cancellationToken);
-        Assert.Equal(Referrers.ReferrersState.ReferrersNotSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.NotSupported, repo.ReferrersState);
         Assert.Equal(firstExpectedManifestBytes, receivedManifestContent);
         Assert.True(oldIndexDeleted);
         Assert.Equal(firstExpectedIndexReferrersBytes, receivedIndexContent);
 
 
         // Second push with referrer tag schema
-        Assert.Equal(Referrers.ReferrersState.ReferrersNotSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.NotSupported, repo.ReferrersState);
         await store.PushAsync(secondExpectedManifestDesc, new MemoryStream(secondExpectedManifestBytes), cancellationToken);
-        Assert.Equal(Referrers.ReferrersState.ReferrersNotSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.NotSupported, repo.ReferrersState);
         Assert.Equal(secondExpectedManifestBytes, receivedManifestContent);
         Assert.True(firstIndexDeleted);
         Assert.Equal(secondExpectedIndexReferrersBytes, receivedIndexContent);
@@ -460,9 +460,9 @@ public async Task ManifestStore_PushAsyncWithSubjectAndReferrerNotSupportedWitho
         var cancellationToken = new CancellationToken();
         var store = new ManifestStore(repo);
 
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
         await store.PushAsync(expectedIndexManifestDesc, new MemoryStream(expectedIndexManifestBytes), cancellationToken);
-        Assert.Equal(Referrers.ReferrersState.ReferrersNotSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.NotSupported, repo.ReferrersState);
         Assert.Equal(expectedIndexManifestBytes, receivedIndexManifestContent);
         Assert.Equal(expectedIndexReferrersBytes, receivedIndexReferrersContent);
     }
@@ -547,9 +547,9 @@ public async Task ManifestStore_PushAsyncWithSubjectAndNoUpdateRequired()
         var cancellationToken = new CancellationToken();
         var store = new ManifestStore(repo);
 
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
         await store.PushAsync(expectedManifestDesc, new MemoryStream(expectedManifestBytes), cancellationToken);
-        Assert.Equal(Referrers.ReferrersState.ReferrersNotSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.NotSupported, repo.ReferrersState);
         Assert.Equal(expectedManifestBytes, receivedManifestContent);
     }
 }
diff --git a/tests/OrasProject.Oras.Tests/Remote/ReferrersTest.cs b/tests/OrasProject.Oras.Tests/Remote/ReferrersTest.cs
@@ -230,7 +230,7 @@ public void ApplyReferrerChanges_ShouldNotAddNewDuplicateReferrers()
     [Fact]
     public void ApplyReferrerChanges_ShouldNotKeepOldEmptyReferrers()
     {
-        var emptyDesc1 = Descriptor.EmptyDescriptor();
+        var emptyDesc1 = Descriptor.ZeroDescriptor();
         Descriptor? emptyDesc2 = null;
         var newDescriptor = RandomDescriptor();
 
@@ -261,7 +261,7 @@ public void ApplyReferrerChanges_ShouldNotKeepOldEmptyReferrers()
     public void ApplyReferrerChanges_NoUpdateWhenOldAndNewReferrersAreEmpty()
     {
         var oldReferrers = new List<Descriptor>();
-        var referrerChange = new Referrers.ReferrerChange(Descriptor.EmptyDescriptor(), Referrers.ReferrerOperation.ReferrerAdd);
+        var referrerChange = new Referrers.ReferrerChange(Descriptor.ZeroDescriptor(), Referrers.ReferrerOperation.ReferrerAdd);
 
         var (updatedReferrers, updateRequired) = Referrers.ApplyReferrerChanges(oldReferrers, referrerChange);
         Assert.Empty(updatedReferrers); 

diff --git a/tests/OrasProject.Oras.Tests/Remote/RepositoryTest.cs b/tests/OrasProject.Oras.Tests/Remote/RepositoryTest.cs
@@ -2549,35 +2549,35 @@ public async Task Repository_MountAsync_Fallback_GetContentError()
     public void SetReferrersState_ShouldSet_WhenInitiallyUnknown()
     {
         var repo = new Repository("localhost:5000/test2");
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
-        repo.SetReferrersState(Referrers.ReferrersState.ReferrersSupported);
-        Assert.Equal(Referrers.ReferrersState.ReferrersSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
+        repo.ReferrersState = Referrers.ReferrersState.Supported;
+        Assert.Equal(Referrers.ReferrersState.Supported, repo.ReferrersState);
     }
 
     [Fact]
     public void SetReferrersState_ShouldThrowException_WhenChangingAfterSet()
     {
         var repo = new Repository("localhost:5000/test2");
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
-        repo.SetReferrersState(Referrers.ReferrersState.ReferrersSupported);
-        Assert.Equal(Referrers.ReferrersState.ReferrersSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
+        repo.ReferrersState = Referrers.ReferrersState.Supported;
+        Assert.Equal(Referrers.ReferrersState.Supported, repo.ReferrersState);
 
         var exception = Assert.Throws<ReferrersStateAlreadySetException>(() =>
-            repo.SetReferrersState(Referrers.ReferrersState.ReferrersNotSupported)
+            repo.ReferrersState = Referrers.ReferrersState.NotSupported
         );
 
-        Assert.Equal("current referrers state: ReferrersSupported, latest referrers state: ReferrersNotSupported", exception.Message);
+        Assert.Equal("current referrers state: Supported, latest referrers state: NotSupported", exception.Message);
     }
 
     [Fact]
     public void SetReferrersState_ShouldNotThrowException_WhenSettingSameValue()
     {
         var repo = new Repository("localhost:5000/test2");
-        Assert.Equal(Referrers.ReferrersState.ReferrersUnknown, repo.ReferrersState);
-        repo.SetReferrersState(Referrers.ReferrersState.ReferrersSupported);
-        Assert.Equal(Referrers.ReferrersState.ReferrersSupported, repo.ReferrersState);
+        Assert.Equal(Referrers.ReferrersState.Unknown, repo.ReferrersState);
+        repo.ReferrersState = Referrers.ReferrersState.Supported;
+        Assert.Equal(Referrers.ReferrersState.Supported, repo.ReferrersState);
 
-        var exception = Record.Exception(() => repo.SetReferrersState(Referrers.ReferrersState.ReferrersSupported));
+        var exception = Record.Exception(() => repo.ReferrersState = Referrers.ReferrersState.Supported);
         Assert.Null(exception);
     }
 }