Skip to content

Make ResourceGraphBuilder extensible #1099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Khaos66 opened this issue Nov 1, 2021 · 7 comments
Closed

Make ResourceGraphBuilder extensible #1099

Khaos66 opened this issue Nov 1, 2021 · 7 comments
Labels

Comments

@Khaos66
Copy link

Khaos66 commented Nov 1, 2021

Is your feature request related to a problem? Please describe.
I need to make a resource type accessable through JADNC that is part of a third party lib. So I can't add AttrAttribute to the properties of this type.

Describe the solution you'd like
It would be nice if it was possible to extend the ResourceGraphBuilder. Maybe through JsonApiResourceDefinition?
Sadly, the methods CreateResourceType and GetAttributes are private and sealed off.

Or did I miss another way to do this?

@bart-degreed
Copy link
Contributor

bart-degreed commented Nov 2, 2021

Hi @Khaos66, thanks for asking.

I agree that ResourceGraphBuilder being non-sealed without any virtual members is not very useful. We could open it up for extension, but in your case, that's not going to solve your problem for two reasons:

  • Building the graph requires to set ResourceFieldAttribute.Property, whose setter is internal.
  • Resources must implement IIdentifiable, which requires the 3rd party library to have a dependency on JsonApiDotNetCore.

The way to solve this is by wrapping the type from the 3rd party library. I've attached a sample project that wraps System.Version. You can run it with F5, which shows sample data.

WrappedThirdPartyResourceDemoApi.zip

For convenience, I'm adding the wrapper here as text too:

[Resource("versions")]
public sealed class VersionResource : Identifiable<string>
{
    private Version _innerVersion = new();

    public override string Id
    {
        get => _innerVersion.ToString();
        set => _innerVersion = value == null ? new Version() : Version.Parse(value);
    }

    [Attr]
    public int Major
    {
        get => _innerVersion.Major;
        set => _innerVersion = new Version(value, _innerVersion.Minor);
    }

    [Attr]
    public int Minor
    {
        get => _innerVersion.Minor;
        set => _innerVersion = new Version(_innerVersion.Major, value);
    }
}

when running the project, it requests:

GET http://localhost:24883/releases?include=version

Which returns:

{
  "links": {
    "self": "http://localhost:24883/releases?include=version",
    "first": "http://localhost:24883/releases?include=version"
  },
  "data": [
    {
      "type": "releases",
      "id": "1",
      "attributes": {
        "title": "JsonApiDotNetCore"
      },
      "relationships": {
        "version": {
          "links": {
            "self": "http://localhost:24883/releases/1/relationships/version",
            "related": "http://localhost:24883/releases/1/version"
          },
          "data": {
            "type": "versions",
            "id": "1.2"
          }
        }
      },
      "links": {
        "self": "http://localhost:24883/releases/1"
      }
    }
  ],
  "included": [
    {
      "type": "versions",
      "id": "1.2",
      "attributes": {
        "major": 1,
        "minor": 2
      },
      "links": {
        "self": "http://localhost:24883/versions/1.2"
      }
    }
  ]
}

Hope that helps.

@Khaos66
Copy link
Author

Khaos66 commented Nov 2, 2021

@bart-degreed Thank you for this comprehensive answer!
I was aware that I would have to implement IIdentifiable with a new sub-class.
The wrapper Properties are ok. But I don't like them...

In my oppinion something like this would be preverable:

public class VersionDefinition : JsonApiResourceDefinition<Version, string>
{
    public override void OnBuildAttributes(IResourceGraphBuilder builder)
    {
        // Attr will create a new instance of AttrAttribute
        builder.Attr(nameof(Version.Major), typeof(Version).GetProperty("Major"));
        builder.Attr(nameof(Version.Minor), typeof(Version).GetProperty("Minor"));
    }
}

Is this clear enough?

EDIT: OnBuildAttributes maybe is too much... OnBuildGraph should be enough to define Attr, Relationships and so on...

@bart-degreed
Copy link
Contributor

That's simply not possible, because each resource must implement IIdentifiable. It is a compiler/runtime constraint that makes it impossible to add an interface at runtime to an already compiled type.

@Khaos66
Copy link
Author

Khaos66 commented Nov 2, 2021

@bart-degreed I'm sorry. I was too lazy and didn't explain it well enough...

Of course it is not possible without a class that implements IIdentifiable. I already have a sub-class that adds some custom properties, so thats not my point.

Here is my actual use case:

using JsonApiDotNetCore.Resources;
using Microsoft.AspNetCore.Identity;
using System.ComponentModel.DataAnnotations.Schema;

[Resource("users")]
public sealed class UserResource : IdentityUser, IIdentifiable<string>
{
    [NotMapped]
    public string LocalId { get; set; }

    [NotMapped]
    public string StringId { get => Id; set => Id = value; }

    [Attr]
    [ProtectedPersonalData]
    public string GivenName { get; set; }

    [Attr]
    [ProtectedPersonalData]
    public string FamilyName { get; set; }
}

IdentityUser.UserName and IdentityUser.Email are not accessable this way =(
In this very example it would be possible to override the property and add AttrAttribute because IdentityUser.UserName is virtual (as I just learned from MSDN =D)

But sometimes this is not the case. And in this case I think it would be nice to be able the define additional Properties via JsonApiResourceDefinition. Do you understand now?
Example:

public class UserDefinition : JsonApiResourceDefinition<UserResource, string>
{
    public override void OnBuildGraph(IResourceGraphBuilder builder)
    {
        // Add missing properties from IdentityUser to UserResource graph
        builder.Attr(nameof(IdentityUser.UserName), typeof(IdentityUser).GetProperty("UserName"));
        builder.Attr(nameof(IdentityUser.Email), typeof(IdentityUser).GetProperty("Email"));
    }
}

In fact, you're doing this already in ResourceGraphBuilder.GetAttributes with the ID property here

@bart-degreed
Copy link
Contributor

Thanks for the clarification, makes sense.

We'd like to solve this using a fluent API in the future. Adding that to IResourceDefinition is not the best place, because it is used for handling a request (and building the resource graph occurs once at startup). It also introduces a chicken-and-egg problem: on startup, custom resource definitions can be added to the IoC container after calling services.AddJsonApi(), but we'd be too late executing their OnBuildGraph method.

If I understand correctly, you're currently unblocked. Therefore I'd prefer to not open up ResourceGraphBuilder at this time because then we'll likely need to make breaking changes when implementing the fluent API.

@Khaos66
Copy link
Author

Khaos66 commented Nov 3, 2021

Ok. I don't have the insights that you have ;) I get your point.
Fluent API is the word I was looking for. Thanks!

In that case you already have this on your roadmap and I'm happy ;)

@Khaos66 Khaos66 closed this as completed Nov 3, 2021
@bart-degreed
Copy link
Contributor

Glad to help :)

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

No branches or pull requests

2 participants