ApiResource default constructor leads to unexpected behavior

[giggio]

These 2 snippets produce different results:

new ApiResource("foo");

new ApiResource()
{
    Name = "foo"
};

This is because setting the property Name does not add the scope to the scope list, but the parameterized constructor does create the Scope list. See

IdentityServer4/src/IdentityServer4/Models/ApiResource.cs

Line 43 in 305347e

Scopes.Add(new Scope(name, displayName));

This is not idiomatic C#, and will generate all sorts of confusion, as people will not imagine that the two snippets produce different results. I suggest you make the default constructor private.

Also, the docs do not mention the difference. See https://identityserver4.readthedocs.io/en/release/quickstarts/1_client_credentials.html

[leastprivilege]

What shall we do with this? Change behavior / add docs? breaking change?

[brockallen]

@giggio suggestions?

[giggio]

It depends on your release strategy. If you are planning on keeping evolving IS, and release versions 5, 6, etc, following semver, than I would deprecate the parameterless constructor on version 4 (with a message warning for the problem), and remove it on version 5. At the same time, post the problem on the docs, alerting people for the problem.
It would also be a good idea to make the Name property read only, to avoid confusion, along with DisplayName.
Whatever way you decide, try to leave only one way to do things, to avoid confusion. I am not sure my suggestion above follows your plans to IS, so maybe you will approach it differently. But make it so that there is only one way to do it. I spent a lot of time trying to figure out why I had a problem, the current way things are set makes it harder to use the library.

[brockallen]

I think what was meant was how to make a nicer API that allows both a convenient one-liner to do the simple common scenario, and yet still allow the more complex configuration.

As a bit of background, we originally chose the ctor approach for discoverability, rather than a static factory style API.

[brockallen]

We discussed and we like the ctor approach. We'll improve the docs and see if that helps.

[agilenut]

I personally don't believe that the constructor should generate a scope for you unless you explicitly tell it too via another parameter. We have several APIs that have multiple scopes. The Name is going to be different than the scope name. If the only constructor available sets the scope by default, we'd have to rip it out after construction and then add additional scopes. That seems messy.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.