From c30d6b23bf2540c427ebb20d00b4ac19665f24e7 Mon Sep 17 00:00:00 2001 From: Patrick Pan Date: Fri, 13 Dec 2024 15:00:04 +1100 Subject: [PATCH] resolve comments Signed-off-by: Patrick Pan --- src/OrasProject.Oras/Oci/Descriptor.cs | 4 +- .../Remote/HttpResponseMessageExtensions.cs | 6 +-- .../Registry/Remote/ManifestStore.cs | 22 +++++++---- .../Registry/Remote/Referrers.cs | 11 +++--- .../Registry/Remote/Repository.cs | 39 +++++++------------ .../Remote/ManifestStoreTest.cs | 30 +++++++------- .../Remote/ReferrersTest.cs | 4 +- .../Remote/RepositoryTest.cs | 24 ++++++------ 8 files changed, 66 insertions(+), 74 deletions(-) diff --git a/src/OrasProject.Oras/Oci/Descriptor.cs b/src/OrasProject.Oras/Oci/Descriptor.cs index 8370aa5..3f35c27 100644 --- a/src/OrasProject.Oras/Oci/Descriptor.cs +++ b/src/OrasProject.Oras/Oci/Descriptor.cs @@ -74,10 +74,10 @@ public static Descriptor Create(Span 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 index af00c48..5e672a5 100644 --- 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. } /// diff --git a/src/OrasProject.Oras/Registry/Remote/ManifestStore.cs b/src/OrasProject.Oras/Registry/Remote/ManifestStore.cs index d74f30f..724354c 100644 --- 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(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(content); - if (imageManifest?.Subject == null) return; + var imageManifest = JsonSerializer.Deserialize(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, /// /// /// - internal async Task<(Descriptor, IList)> PullReferrersIndexList(String referrersTag, CancellationToken cancellationToken = default) + internal async Task<(Descriptor?, IList)> PullReferrersIndexList(String referrersTag, CancellationToken cancellationToken = default) { try { @@ -320,7 +326,7 @@ private async Task UpdateReferrersIndex(Descriptor subject, } catch (NotFoundException) { - return (Descriptor.EmptyDescriptor(), new List()); + return (null, new List()); } } diff --git a/src/OrasProject.Oras/Registry/Remote/Referrers.cs b/src/OrasProject.Oras/Registry/Remote/Referrers.cs index 0e1fbe6..de495c2 100644 --- 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(':', '-'); } /// diff --git a/src/OrasProject.Oras/Registry/Remote/Repository.cs b/src/OrasProject.Oras/Registry/Remote/Repository.cs index 613400f..742dcc6 100644 --- 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; + /// + /// ReferrersState indicates the Referrers API state of the remote repository. + /// ReferrersState can be set only once, otherwise it throws ReferrersStateAlreadySetException. + /// 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; } - /// - /// 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. - /// - /// - /// - 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}"); - } - } - /// /// FetchAsync fetches the content identified by the descriptor. /// diff --git a/tests/OrasProject.Oras.Tests/Remote/ManifestStoreTest.cs b/tests/OrasProject.Oras.Tests/Remote/ManifestStoreTest.cs index e8fd70b..0b1ac8c 100644 --- 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 index 5db44fd..394f272 100644 --- 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(); - 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 index 6aae7e1..b893c34 100644 --- 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(() => - 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); } }