Skip to content

Attribute Naming Conventions #293

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
jaredcnance opened this issue Jun 10, 2018 · 6 comments
Closed

Attribute Naming Conventions #293

jaredcnance opened this issue Jun 10, 2018 · 6 comments

Comments

@jaredcnance
Copy link
Contributor

It would be great if we could define an app-wide naming convention for attributes and relationships with default options. So, instead of:

[Attr("fooBar")]
public object FooBar { get; set; }

You would specify a convention:

// Startup.cs
options.AttributeNameConvention = NamingConvention.CamelCase;

// Model.cs

[Attr]
public object FooBar { get; set; }

A possible interface for AttributeNameConvention might be:

public interface INamingConvention {
  string Convert(string propertyName);
}
@jaredcnance jaredcnance changed the title Support Attribute Naming Conventions Attribute Naming Conventions Jun 10, 2018
@btecu
Copy link
Contributor

btecu commented Sep 9, 2018

Instead of setting the field name straight on the properties or introducing a new setting (keep code simple), we could just use Newtonsoft.Json.Serialization.NamingStrategy.

Somewhat related, Newtonsoft.Json provides enough decorators that it wouldn't be necessary to have our own [Attr], [BelongTo] or [HasMany]. Those can be inferred from types. Simple properties such as primitives, DateTime, Guid and string can be considered [Attr]. Class properties can be [BelongsTo] (one to one) while collections will be [HasMany].

Opting in would be the default and to opt out of serializing a field, [JsonIgnore] (Newtonsoft.Json.JsonIgnoreAttribute) can be used.

ContextGraph along with Newtonsoft.Json should be sufficient to build a serialization / deserialization package that can be isolated from the other EF / Controllers specific codebase.

@btecu
Copy link
Contributor

btecu commented Sep 9, 2018

If overriding the name is a concern, [JsonProperty] (Newtonsoft.Json.JsonPropertyAttribute) allows for specific a custom name (overrides the naming strategy).

@jaredcnance
Copy link
Contributor Author

jaredcnance commented Sep 9, 2018

I think using the existing NamingStrategy type would probably be fine, we already have a pretty hard dependency on Newtonsoft, so I don't think there's a problem there. Also, in case you weren't aware, we already allow users to specify JsonSerializerSettings for serialization of complex types. I need to look a little closer to see if this would be the right place to specify this behavior or if we should have an additional configuration value for NamingStrategy.

public JsonSerializerSettings SerializerSettings { get; } = new JsonSerializerSettings()
{
NullValueHandling = NullValueHandling.Ignore,
ContractResolver = new DasherizedResolver()
};

One thing to remember is that the existing attributes provide more options than just name overrides. I've enumerated these other configuration properties below:

  • Attr: publicName, isImmutable, isFilterable, isSortable
  • HasOne / HasMany: publicName, documentLinks, canInclude, withForeignKey

So, while I think our target should be enabling resource definitions that do not require these attributes, I don't think we should dispense with them altogether.

Also, inferring relationships based on types can get tricky. This is not a safe assumption:

Class properties can be [BelongsTo] (one to one) while collections will be [HasMany]

There are some reference (class) types that would be considered "simple" types (e.g. String) so that would require whitelisting simple types. Also, we support complex attributes, for example:

{
  "data": {
    "attributes": {
       "key" : {
         "nested-key": "nested-value"
       }
    }
  }
}

An alternative to this is to try to discover all resources and then determine their relationships by looking for properties where the property type is a registered type on the graph (or the property type is an enumerable of a registered type -- List<T>). I think this is feasible, but probably out-of-scope for this first pass.

This is an interesting idea:

Opting in would be the default and to opt out of serializing a field, [JsonIgnore]

But, I have a couple reservations here:

  • Types may need to be serialized in contexts other than json:api request/responses. Maybe this is ok.
  • This is not the only way to determine whether or not a type should be serialized in Newtonsoft.Json. There are some features in Newtonsoft that we wouldn't support and I could see this causing confusion:

I think it may be confusing if users try to use established Newtonsoft.Json features and then they just don't work because we aren't simply doing JsonConvert.DeserializeObject<T>(...).
We don't actually deserialize directly using Newtonsoft. It looks more like:

  • Deserialization:
    stringJsonConvert.DeserializeDocument → extract context and create the target type (Activator.CreateInstance)

  • Serialization:
    resource → create Document which contains ResourceObjects which are dictionaries with string keys → JsonConvert.Serializestring

public class ResourceObject : ResourceIdentifierObject
{
[JsonProperty("attributes")]
public Dictionary<string, object> Attributes { get; set; }
[JsonProperty("relationships", NullValueHandling = NullValueHandling.Ignore)]
public Dictionary<string, RelationshipData> Relationships { get; set; }
}

Because of these reasons, I believe we should keep the existing attributes. They can be used as overrides when there are exceptions to the established convention, or extra configuration is required that is specific to the attribute (e.g isFilterable = false).


For this iteration users should be able to change models from:

public class Article : Identifiable {
  [Attr("name")]
  public string Name { get; set; }

  [HasMany("authors")]
  public List<Person> Authors { get; set; }
}

To:

public class Article : Identifiable {
  public string Name { get; set; }

  [HasMany("authors")]
  public List<Person> Authors { get; set; }
}

And after additional planning and design, I think we can achieve the following. This will require significant changes to how the ContextGraph is constructed.

public class Article : Identifiable {
  public string Name { get; set; }
  public List<Person> Authors { get; set; }
}

@jaredcnance
Copy link
Contributor Author

jaredcnance commented Sep 9, 2018

With respect to removing attributes, this can be done using the ResourceDefinition. Currently undocumented, but available since v2.3.4 (#297 via #324).

public class RequestFilteredResource : ResourceDefinition<Model>
{
private readonly bool _isAdmin;
// this constructor will be resolved from the container
// that means you can take on any dependency that is also defined in the container
public RequestFilteredResource(bool isAdmin)
{
_isAdmin = isAdmin;
}
// Called once per filtered resource in request.
protected override List<AttrAttribute> OutputAttrs()
=> _isAdmin
? Remove(m => m.AlwaysExcluded)
: Remove(m => new { m.AlwaysExcluded, m.Password }, from: base.OutputAttrs());
}

Or create two distinct models (a resource and db entity), mapped at the service layer. Currently undocumented, but available since v2.4.0 (#112 via #344, #352). In the example, CourseResource is mapped to CourseEntity which allows you to only define properties on the resource that should actually be exposed.

services.AddAutoMapper();
services.AddScoped<IResourceMapper, AutoMapperAdapter>();
services.AddScoped<IResourceService<CourseResource, int>, EntityResourceService<CourseResource, CourseEntity, int>>();

@btecu
Copy link
Contributor

btecu commented Sep 9, 2018

Not sure what the difference between name and publicName is but the other ones, isImmutable, isFilterable and isSortable are only needed for the other part of the project, the one that involves controllers. I'm not sure if there's a way where we can include those extensions as part of say the Controller package since the serialization wouldn't need to have knowledge of those.

The idea with [JsonIgnore] was that the field would not make it in the serialization / deserialization. Using instead ShouldSerialize would cause the same result?

I'm guessing it's not feasible to go from class to Json and from string to class without going through the intermediate Document but if it was, it would make things easier.

@jaredcnance
Copy link
Contributor Author

jaredcnance commented Sep 9, 2018

difference between name and publicName

They are the same. The property is called PublicAttributeName.

are only needed for the other part of the project, the one that involves controllers

This is the main part of the project. If all you want is serialization I recommend one of the other .Net libraries. Serialization is a small component of this project and I don't think it will be pulled out into a separate package anytime soon (unless someone else is willing to do it). I personally, don't have the motivation to separate it for the sake of separation. FWIW we do have plans to separate the ORM layer in an upcoming release.

the field would not make it in the serialization / deserialization

Yes, I'm aware of what JsonIgnore does. My point is we would only support a subset of Newtonsoft's features, but users will likely think we support all of them because they will think their model is getting passed directly to the Newtonsoft serializer. So, if they tried to use ShouldSerialize to control the behavior, it just wouldn't work. Remember we’re serializing a Dictionary<string, object> through Newtonsoft, not an instance decorated with JsonIgnore.

But, with all that said, I think I’m in agreement that we should probably honor JsonIgnore attributes if they are present and it would be better than introducing a new attribute.

jaredcnance added a commit that referenced this issue Oct 3, 2018
Feat/#293: Naming Conventions, Improvements to AutoDiscovery, and ResourceDefinition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants