[FEATURE] update ACL Token `Read` endpoint to add `expanded` parameter by MohamedM216 · Pull Request #601 · G-Research/consuldotnet

MohamedM216 · 2026-03-10T05:50:50Z

Description

update ACL Token Read endpoint to add expnded parameter

Related Issues

#531

Additional Context

Checklist

Please make sure to check the following before submitting your pull request:

Did you read the contributing guidelines?
Did you update the docs?
Did you write any tests to validate this change?

…test

marcin-krystianc

HI @MohamedM216,

look at the example of a response when the expanded=true (https://developer.hashicorp.com/consul/api-docs/acl/tokens#sample-response-1). See, there are two new fields, ExpandedPolicies and ExpandedRoles.
The test should assert that these fields are filled in, when expanded=true and null when expanded=false.

…token-read

MohamedM216 · 2026-03-10T14:25:53Z

ExpandedPolicies and ExpandedRoles don't exist in QueryResult<TokenEntry> or PolicyLink. PolicyLink only has ID and Name. Any ideas?

…token-read

marcin-krystianc · 2026-03-18T09:04:46Z

+        public TokenEntry(string accessorId, string description, PolicyLink[] policies, RoleLink[] roles, ServiceIdentity[] serviceIdentities, ExpandedPolicy[] expandedPolicies, ExpandedRole[] expandedRoles, string[] namespaceDefaultPolicyIDs, string[] namespaceDefaultRoleIDs, string agentACLDefaultPolicy, string agentACLDownPolicy, string resolvedByAgent)
+        {
+            AccessorID = accessorId;
+            Description = description;
+            Policies = policies;
+            Roles = roles;
+            ServiceIdentities = serviceIdentities;
+            ExpandedPolicies = expandedPolicies;
+            ExpandedRoles = expandedRoles;
+            NamespaceDefaultPolicyIDs = namespaceDefaultPolicyIDs;
+            NamespaceDefaultRoleIDs = namespaceDefaultRoleIDs;
+            AgentACLDefaultPolicy = agentACLDefaultPolicy;
+            AgentACLDownPolicy = agentACLDownPolicy;
+            ResolvedByAgent = resolvedByAgent;
+        }


I don't think we need to add this constructor? It just adds noise to the codebase.

marcin-krystianc · 2026-03-18T09:04:56Z

+        public ExpandedRole(string id, string name, string description, PolicyLink[] policies, ServiceIdentity[] serviceIdentities)
+        {
+            ID = id;
+            Name = name;
+            Description = description;
+            Policies = policies;
+            ServiceIdentities = serviceIdentities;
+        }
+    }


I don't think we need to add this constructor? It just adds noise to the codebase.

marcin-krystianc · 2026-03-18T09:08:42Z

+        public ExpandedPolicy(string id, string name, string description, string rules, string[] datacenters)
+        {
+            ID = id;
+            Name = name;
+            Description = description;
+            Rules = rules;
+            Datacenters = datacenters;
+        }


I don't think we need to add this constructor? It just adds noise to the codebase.

marcin-krystianc · 2026-03-18T09:24:43Z

+            // create test policy
+            var policyEntry = new PolicyEntry
+            {
+                Name = "TestExpandedPolicy-" + Guid.NewGuid().ToString().Substring(0, 8),


Please use KVTest.GenerateTestKeyName() for generating random names (we want to have a consistent way of generating random names).

marcin-krystianc · 2026-03-19T11:45:55Z

+//  </copyright>
+// -----------------------------------------------------------------------
+
+using System.Data;


it is not used, please remove:

Suggested change

using System.Data;

marcin-krystianc

Please improve assertions where applicable (e.g. Assert.NotEmpty instead of Assert.NotNull)

marcin-krystianc · 2026-03-19T11:58:09Z

+                // when expanded=false
+                var unexpandedRead = await _client.Token.Read(accessorId, false, QueryOptions.Default);
+                Assert.NotNull(unexpandedRead.Response.Policies);
+                Assert.Equal(unexpandedRead.Response.Policies.FirstOrDefault().Name, policyEntry.Name);


xUnit convention is Assert.Equal(expected, actual):

Suggested change

Assert.Equal(unexpandedRead.Response.Policies.FirstOrDefault().Name, policyEntry.Name);

Assert.Equal(policyEntry.Name, unexpandedRead.Response.Policies.FirstOrDefault().Name);

marcin-krystianc · 2026-03-19T11:59:06Z

+                Assert.NotNull(expandedRead.Response.AgentACLDownPolicy);
+                Assert.NotNull(expandedRead.Response.ResolvedByAgent);
+                Assert.NotNull(expandedRead.Response.ExpandedPolicies);
+                Assert.Equal(expandedRead.Response.ExpandedPolicies.FirstOrDefault().Rules, policyEntry.Rules);


` xUnit convention is Assert.Equal(expected, actual):

No need to use the FirstOrDefault() form (it would throw confusing exception, it ExpandedPolicies was empty)

Suggested change

Assert.Equal(expandedRead.Response.ExpandedPolicies.FirstOrDefault().Rules, policyEntry.Rules);

Assert.Equal(policyEntry.Rules, expandedRead.Response.ExpandedPolicies.First().Rules);

marcin-krystianc · 2026-03-19T11:59:26Z

+                Assert.NotNull(expandedRead.Response.ExpandedPolicies);
+                Assert.Equal(expandedRead.Response.ExpandedPolicies.FirstOrDefault().Rules, policyEntry.Rules);
+                Assert.NotNull(expandedRead.Response.ExpandedRoles);
+                Assert.Equal(expandedRead.Response.ExpandedRoles.FirstOrDefault().Name, roleEntry.Name);


xUnit convention is Assert.Equal(expected, actual):

Suggested change

Assert.Equal(expandedRead.Response.ExpandedRoles.FirstOrDefault().Name, roleEntry.Name);

Assert.Equal(roleEntry.Name, expandedRead.Response.ExpandedRoles.FirstOrDefault().Name);

marcin-krystianc · 2026-03-19T12:07:31Z

+                Assert.NotNull(expandedRead.Response.AgentACLDefaultPolicy);
+                Assert.NotNull(expandedRead.Response.AgentACLDownPolicy);
+                Assert.NotNull(expandedRead.Response.ResolvedByAgent);
+                Assert.NotNull(expandedRead.Response.ExpandedPolicies);


Slightly better (stricter) check:

Suggested change

Assert.NotNull(expandedRead.Response.ExpandedPolicies);

Assert.NotEmpty(expandedRead.Response.ExpandedPolicies);

marcin-krystianc · 2026-03-19T12:08:20Z

+        public async Task Token_ReadExpanded()
+        {
+            var cutOffVersion = SemanticVersion.Parse("1.12.0");
+            Skip.If(AgentVersion < cutOffVersion, $"Current version is {AgentVersion}, but `Namespaces` is only supported from Consul {cutOffVersion}");


Error message is not accurate, please update it.

- improve assertions: use Assert.NotEmpty instead of Assert.NotNull - better consul versions exclusion message

marcin-krystianc

Looks good now, thank you!

…token-read

MohamedM216 · 2026-03-20T16:50:42Z

        Task<WriteResult<bool>> Delete(string id, WriteOptions q, CancellationToken ct = default);
        Task<QueryResult<TokenEntry>> Read(string id, CancellationToken ct);
        Task<QueryResult<TokenEntry>> Read(string id);
-        Task<QueryResult<TokenEntry>> Read(string id, QueryOptions q, CancellationToken ct = default);


Due to losing this API, now during the building process, we get this:

/home/mohammad/programs/mlh-fellowship/projects/consuldotnet/consuldotnet/Consul/PublicAPI/PublicAPI.Shipped.txt(3013,1): error RS0017: Symbol 'Consul.ITokenEndpoint.Read(string id, Consul.QueryOptions q, System.Threading.CancellationToken ct = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task<Consul.QueryResult<Consul.TokenEntry>>' is part of the declared API, but is either not public or could not be found (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md) /home/mohammad/programs/mlh-fellowship/projects/consuldotnet/consuldotnet/Consul/PublicAPI/PublicAPI.Shipped.txt(4099,1): error RS0017: Symbol 'Consul.Token.Read(string id, Consul.QueryOptions queryOptions, System.Threading.CancellationToken ct = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.Task<Consul.QueryResult<Consul.TokenEntry>>' is part of the declared API, but is either not public or could not be found (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md)

It's a breaking change and we should keep it in Shipped.txt. So the only way I see is to add the expanded parameter to QueryOptions, although QueryOptions is generic and expanded does only belong to ACL token Read endpoints, then we won't have any breaking changes and won't have to add any new overloads as well?
Also it's good to mention that we will face the same issue in #592

Please let me know your thoughts.
Thanks!

See here how to handle this (dotnet/roslyn-analyzers#5058):

You should copy the entry for your change from PublicAPI.Shipped.txt and then put that entry in PublicAPI.Unshipped.txt with REMOVED prefix.

marcin-krystianc · 2026-03-23T09:38:15Z

+                var list = await _client.Token.List();
+                var testToken = list.Response.FirstOrDefault(t => t.Description == "Expansion Test Token");
+                if (testToken != null)
+                {
+                    await _client.Token.Delete(testToken.AccessorID);
+                }


This is unecessarily complicated. Maybe let's get rid of try/finally and just assume test always completes normally, so no exception handling is needed (which is generally true). There is no need to complicate the test code to handle edge cases.

Once the try/finally is removed , then we don't need to look for a token to delete since we already have its AccessorID.

…token-read

marcin-krystianc

I think this is a last remark, looks good otherwise!

marcin-krystianc · 2026-03-24T09:56:02Z

+        [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
+        public string[] NamespaceDefaultPolicyIDs { get; set; }
+        [JsonProperty(NullValueHandling = NullValueHandling.Ignore)]
+        public string[] NamespaceDefaultRoleIDs { get; set; }


I would rather not add properties for which we do not have proper testing, it is better not to add them blindly (I'm not even sure when and how these would be non null):

Suggested change

[JsonProperty(NullValueHandling = NullValueHandling.Ignore)]

public string[] NamespaceDefaultPolicyIDs { get; set; }

[JsonProperty(NullValueHandling = NullValueHandling.Ignore)]

public string[] NamespaceDefaultRoleIDs { get; set; }

update acl token Read endpoint to add expnded parameter and add unit …

ab4b32f

…test

MohamedM216 mentioned this pull request Mar 10, 2026

[FEATURE] update ACL Token Read endpoint to add expnded parameter #590

Closed

3 tasks

marcin-krystianc reviewed Mar 10, 2026

View reviewed changes

Merge branch 'master' of github.com:G-Research/consuldotnet into acl-…

f6bda2c

…token-read

MohamedM216 force-pushed the acl-token-read branch from be4536f to f6bda2c Compare March 10, 2026 14:08

MohamedM216 added 3 commits March 17, 2026 17:16

add ExpandedPolicies and ExpandedRoles to TokenEntry

cd835b3

update tests: Token_ReadExpanded

2a08bd6

Merge branch 'master' of github.com:G-Research/consuldotnet into acl-…

8b908d1

…token-read

marcin-krystianc reviewed Mar 18, 2026

View reviewed changes

polishing + skip test with older versions

6fceff1

MohamedM216 force-pushed the acl-token-read branch from 992ff66 to 6fceff1 Compare March 19, 2026 03:39

increase the cutoffversion

44c89ff

marcin-krystianc reviewed Mar 19, 2026

View reviewed changes

polishing

23690f7

- improve assertions: use Assert.NotEmpty instead of Assert.NotNull - better consul versions exclusion message

MohamedM216 requested a review from marcin-krystianc March 20, 2026 08:32

marcin-krystianc previously approved these changes Mar 20, 2026

View reviewed changes

MohamedM216 added 2 commits March 20, 2026 16:39

Merge branch 'master' of github.com:G-Research/consuldotnet into acl-…

56decd4

…token-read

add public api entries to unshipped file

13a1b58

MohamedM216 dismissed marcin-krystianc’s stale review via 13a1b58 March 20, 2026 14:57

Merge branch 'master' of github.com:G-Research/consuldotnet into acl-…

b3824cd

…token-read

MohamedM216 commented Mar 20, 2026

View reviewed changes

MohamedM216 requested a review from marcin-krystianc March 20, 2026 16:51

fix: update PublicAPI.Unshipped.txt population

a72e053

marcin-krystianc reviewed Mar 23, 2026

View reviewed changes

MohamedM216 added 2 commits March 24, 2026 10:17

simplify test

c53fbff

Merge branch 'master' of github.com:G-Research/consuldotnet into acl-…

716502c

…token-read

MohamedM216 requested a review from marcin-krystianc March 24, 2026 08:29

marcin-krystianc reviewed Mar 24, 2026

View reviewed changes

MohamedM216 added 2 commits March 24, 2026 16:19

remove unneeded properties

8439b95

format

90b3886

MohamedM216 force-pushed the acl-token-read branch from 2a22818 to 90b3886 Compare March 24, 2026 14:22

MohamedM216 requested a review from marcin-krystianc March 24, 2026 14:22

marcin-krystianc changed the title ~~[FEATURE] update ACL Token Read endpoint to add expnded parameter~~ [FEATURE] update ACL Token Read endpoint to add expanded parameter Mar 24, 2026

marcin-krystianc approved these changes Mar 24, 2026

View reviewed changes

marcin-krystianc merged commit 345a236 into G-Research:master Mar 24, 2026
120 checks passed

marcin-krystianc mentioned this pull request Apr 7, 2026

feature: updated acl-token implementation #532

Closed

3 tasks

	Assert.Equal(unexpandedRead.Response.Policies.FirstOrDefault().Name, policyEntry.Name);
	Assert.Equal(policyEntry.Name, unexpandedRead.Response.Policies.FirstOrDefault().Name);

	Assert.Equal(expandedRead.Response.ExpandedPolicies.FirstOrDefault().Rules, policyEntry.Rules);
	Assert.Equal(policyEntry.Rules, expandedRead.Response.ExpandedPolicies.First().Rules);

	Assert.Equal(expandedRead.Response.ExpandedRoles.FirstOrDefault().Name, roleEntry.Name);
	Assert.Equal(roleEntry.Name, expandedRead.Response.ExpandedRoles.FirstOrDefault().Name);

	Assert.NotNull(expandedRead.Response.ExpandedPolicies);
	Assert.NotEmpty(expandedRead.Response.ExpandedPolicies);

Conversation

MohamedM216 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Additional Context

Checklist

Uh oh!

marcin-krystianc left a comment

Choose a reason for hiding this comment

Uh oh!

MohamedM216 commented Mar 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcin-krystianc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcin-krystianc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcin-krystianc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MohamedM216 commented Mar 10, 2026 •

edited

Loading