Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use DisplayAttribute methods when getting name and description #1313

Merged
merged 3 commits into from
Feb 13, 2021
Merged

Use DisplayAttribute methods when getting name and description #1313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04b10ec
Use DisplayAttribute methods when getting name and description
wazzamatazz Feb 12, 2021
b1da211
Check for null before calling DisplayAttribute methods
wazzamatazz Feb 12, 2021
3aa50c8
Get name/description and then assign if not null
wazzamatazz Feb 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/NJsonSchema.Tests/Generation/AttributeGenerationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,21 @@ public async Task When_display_attribute_is_available_then_name_and_description_
Assert.Equal("Bar", property.Description);
}

[Fact]
public async Task When_display_attribute_with_resource_type_is_available_then_name_and_description_are_read()
{
//// Arrange


//// Act
var schema = JsonSchema.FromType<AttributeTestClass>();
var property = schema.Properties["DisplayWithResource"];

//// Assert
Assert.Equal(AttributeGenerationTestsResources.AttributeGenerationsTests_Name, property.Title);
Assert.Equal(AttributeGenerationTestsResources.AttributeGenerationsTests_Description, property.Description);
}

[Fact]
public async Task When_description_attribute_is_available_then_description_are_read()
{
Expand Down Expand Up @@ -190,6 +205,13 @@ public class AttributeTestClass

[ReadOnly(true)]
public bool ReadOnly { get; set; }

[Display(
ResourceType = typeof(AttributeGenerationTestsResources),
Name = nameof(AttributeGenerationTestsResources.AttributeGenerationsTests_Name),
Description = nameof(AttributeGenerationTestsResources.AttributeGenerationsTests_Description)
)]
public string DisplayWithResource { get; set; }
}

public class ClassWithTypedRange
Expand Down
81 changes: 81 additions & 0 deletions src/NJsonSchema.Tests/Generation/AttributeGenerationTestsResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

126 changes: 126 additions & 0 deletions src/NJsonSchema.Tests/Generation/AttributeGenerationTestsResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
<?xml version="1.0" encoding="utf-8"?>
<root>
<!--
Microsoft ResX Schema

Version 2.0

The primary goals of this format is to allow a simple XML format
that is mostly human readable. The generation and parsing of the
various data types are done through the TypeConverter classes
associated with the data types.

Example:

... ado.net/XML headers & schema ...
<resheader name="resmimetype">text/microsoft-resx</resheader>
<resheader name="version">2.0</resheader>
<resheader name="reader">System.Resources.ResXResourceReader, System.Windows.Forms, ...</resheader>
<resheader name="writer">System.Resources.ResXResourceWriter, System.Windows.Forms, ...</resheader>
<data name="Name1"><value>this is my long string</value><comment>this is a comment</comment></data>
<data name="Color1" type="System.Drawing.Color, System.Drawing">Blue</data>
<data name="Bitmap1" mimetype="application/x-microsoft.net.object.binary.base64">
<value>[base64 mime encoded serialized .NET Framework object]</value>
</data>
<data name="Icon1" type="System.Drawing.Icon, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
<value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
<comment>This is a comment</comment>
</data>

There are any number of "resheader" rows that contain simple
name/value pairs.

Each data row contains a name, and value. The row also contains a
type or mimetype. Type corresponds to a .NET class that support
text/value conversion through the TypeConverter architecture.
Classes that don't support this are serialized and stored with the
mimetype set.

The mimetype is used for serialized objects, and tells the
ResXResourceReader how to depersist the object. This is currently not
extensible. For a given mimetype the value must be set accordingly:

Note - application/x-microsoft.net.object.binary.base64 is the format
that the ResXResourceWriter will generate, however the reader can
read any of the formats listed below.

mimetype: application/x-microsoft.net.object.binary.base64
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.soap.base64
value : The object must be serialized with
: System.Runtime.Serialization.Formatters.Soap.SoapFormatter
: and then encoded with base64 encoding.

mimetype: application/x-microsoft.net.object.bytearray.base64
value : The object must be serialized into a byte array
: using a System.ComponentModel.TypeConverter
: and then encoded with base64 encoding.
-->
<xsd:schema id="root" xmlns="" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:msdata="urn:schemas-microsoft-com:xml-msdata">
<xsd:import namespace="http://www.w3.org/XML/1998/namespace" />
<xsd:element name="root" msdata:IsDataSet="true">
<xsd:complexType>
<xsd:choice maxOccurs="unbounded">
<xsd:element name="metadata">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" />
</xsd:sequence>
<xsd:attribute name="name" use="required" type="xsd:string" />
<xsd:attribute name="type" type="xsd:string" />
<xsd:attribute name="mimetype" type="xsd:string" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="assembly">
<xsd:complexType>
<xsd:attribute name="alias" type="xsd:string" />
<xsd:attribute name="name" type="xsd:string" />
</xsd:complexType>
</xsd:element>
<xsd:element name="data">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
<xsd:element name="comment" type="xsd:string" minOccurs="0" msdata:Ordinal="2" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" msdata:Ordinal="1" />
<xsd:attribute name="type" type="xsd:string" msdata:Ordinal="3" />
<xsd:attribute name="mimetype" type="xsd:string" msdata:Ordinal="4" />
<xsd:attribute ref="xml:space" />
</xsd:complexType>
</xsd:element>
<xsd:element name="resheader">
<xsd:complexType>
<xsd:sequence>
<xsd:element name="value" type="xsd:string" minOccurs="0" msdata:Ordinal="1" />
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="required" />
</xsd:complexType>
</xsd:element>
</xsd:choice>
</xsd:complexType>
</xsd:element>
</xsd:schema>
<resheader name="resmimetype">
<value>text/microsoft-resx</value>
</resheader>
<resheader name="version">
<value>2.0</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="AttributeGenerationsTests_Description" xml:space="preserve">
<value>Description from resource</value>
</data>
<data name="AttributeGenerationsTests_Name" xml:space="preserve">
<value>Name from resource</value>
</data>
</root>
15 changes: 15 additions & 0 deletions src/NJsonSchema.Tests/NJsonSchema.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,19 @@
<ProjectReference Include="..\NJsonSchema\NJsonSchema.csproj" />
</ItemGroup>

<ItemGroup>
<Compile Update="Generation\AttributeGenerationTestsResources.Designer.cs">
<DesignTime>True</DesignTime>
<AutoGen>True</AutoGen>
<DependentUpon>AttributeGenerationTestsResources.resx</DependentUpon>
</Compile>
</ItemGroup>

<ItemGroup>
<EmbeddedResource Update="Generation\AttributeGenerationTestsResources.resx">
<Generator>PublicResXFileCodeGenerator</Generator>
<LastGenOutput>AttributeGenerationTestsResources.Designer.cs</LastGenOutput>
</EmbeddedResource>
</ItemGroup>

</Project>
5 changes: 3 additions & 2 deletions src/NJsonSchema/Generation/JsonSchemaGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,9 +351,10 @@ public virtual void ApplyDataAnnotations(JsonSchema schema, JsonTypeDescription
var contextualType = typeDescription.ContextualType;

dynamic displayAttribute = contextualType.ContextAttributes.FirstAssignableToTypeNameOrDefault("System.ComponentModel.DataAnnotations.DisplayAttribute");
if (displayAttribute != null && displayAttribute.Name != null)
if (displayAttribute != null)
{
schema.Title = displayAttribute.Name;
// GetName returns null if the Name property on the attribute is not specified.
schema.Title = displayAttribute.GetName();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using GetName() a breaking change from what we had before?
Maybe we should add a null check here as well (as before) as title might already be set and gets resetted to null otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetName() will return the value of the Name field if the ResourceType property on the DisplayAttribute is null, so in this circumstance, the behaviour is the same.

If a ResourceType property is specified on the DisplayAttribute, then GetName() will retrieve the value from the ResourceType instead e.g.

public class MyClass {
  // No ResourceType specified: DisplayAttribute.GetName() returns "Some_Property"
  [Display(Name = "Some_Property")]
  public string SomeProperty { get; set; }
}


public class MyOtherClass {
  // ResourceType specified: DisplayAttribute.GetName() returns MyResources.Some_Property
  [Display(ResourceType = typeof(MyResources), Name = "Some_Property")]
  public string SomeProperty { get; set; }
}

Current behaviour does not have a check to see if the title has already been set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed a check to ensure that the Name property is not null before calling GetName()

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a .Name check and know the internals of GetName() I think it would be better to do:

if (displayAttribute != null)
{
     var name = displayAttribute.GetName();
     if (name != null)
     {
         schema.Title = name;
     }
}

this way we only use GetName() and not assume a correlation between Name and GetName() (same for description)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 3aa50c8

}

dynamic defaultValueAttribute = contextualType.ContextAttributes.FirstAssignableToTypeNameOrDefault("System.ComponentModel.DefaultValueAttribute");
Expand Down
9 changes: 7 additions & 2 deletions src/NJsonSchema/Infrastructure/TypeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,14 @@ public static string GetDescription(this CachedType type, DescriptionAttributeTy
else
{
dynamic displayAttribute = attributes.FirstAssignableToTypeNameOrDefault("System.ComponentModel.DataAnnotations.DisplayAttribute");
if (displayAttribute != null && !string.IsNullOrEmpty(displayAttribute.Description))
if (displayAttribute != null)
{
return displayAttribute.Description;
// GetDescription returns null if the Description property on the attribute is not specified.
var description = displayAttribute.GetDescription();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using GetDescription() a breaking change from what we had before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetDescription() will return the value of the Description field if the ResourceType property on the DisplayAttribute is null, so in this circumstance, the behaviour is the same.

If a ResourceType property is specified on the DisplayAttribute, then GetDescription() will retrieve the value from the ResourceType instead which is the intended behaviour of this change e.g.

public class MyClass {
  // No ResourceType specified: DisplayAttribute.GetDescription() returns "Some_Property_Desc"
  [Display(Description = "Some_Property_Desc")]
  public string SomeProperty { get; set; }
}


public class MyOtherClass {
  // ResourceType specified: DisplayAttribute.GetDescription() returns MyResources.Some_Property_Desc
  [Display(ResourceType = typeof(MyResources), Name = "Some_Property_Desc")]
  public string SomeProperty { get; set; }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed a check to ensure that the Description property is not null before calling GetDescription()

if (description != null)
{
return description;
}
}

if (type is ContextualMemberInfo contextualMember)
Expand Down