Conversation
There was a problem hiding this comment.
Pull request overview
This PR bumps MXTires.Microdata to 1.0.4.6 and expands the schema.org model and validation/serialization behavior, while modernizing project/test configuration and updating dependencies.
Changes:
- Extend the schema model with
MemberProgramTierand newPriceSpecificationproperties. - Refactor
Thingfor stricter type validation, add new schema.org properties, and centralize JSON serialization settings. - Modernize the test project to SDK-style and update library target frameworks/dependencies (incl. .NET 8 and Newtonsoft.Json 13.0.4).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Thing.cs | Adds identifier/subjectOf/disambiguatingDescription, centralizes JSON serialization, and refactors type validation. |
| PriceSpecification.cs | Adds schema.org properties like eligibleQuantity, eligibleTransactionVolume, membershipPointsEarned, eligibleMemberTier, and priceCurrency. |
| Intangible/Offer.cs | Updates priceSpecification validation to allow types from the PriceSpecifications namespace. |
| Intangible/MemberProgramTier.cs | Introduces new schema type MemberProgramTier with tierRequirement. |
| MXTires.Microdata.csproj | Updates version/framework targets and bumps Newtonsoft.Json dependency. |
| Tests/MXTires.Microdata.Tests.csproj | Migrates tests to SDK-style net48 with MSTest NuGet packages. |
| Tests/Properties/AssemblyInfo.cs | Removes legacy AssemblyInfo (consistent with SDK-style projects). |
| Products/Product.cs | Removes unused using directives. |
| .gitignore | Ignores .vs/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new TypeValidator("MXTires.Microdata.CreativeWorks", null, new List<Type> { typeof(string), typeof(WebSite) }); | ||
|
|
||
| private static readonly TypeValidator IdentifierValidator = | ||
| new TypeValidator("MXTires.Microdata.Intangible", null, new List<Type> { typeof(string), typeof(PropertyValue) }); |
There was a problem hiding this comment.
IdentifierValidator is constructed with the namespace "MXTires.Microdata.Intangible", which makes the identifier property accept any type in that namespace (e.g., Offer, Brand, etc.). Schema.org identifier should be limited to PropertyValue/Text/URL (and optionally collections of those), so this validator is overly permissive and defeats type checking. Prefer an explicit acceptable-type list (e.g., string/PropertyValue/(List/IList of those), and/or Uri) rather than a namespace-based validator here.
| new TypeValidator("MXTires.Microdata.Intangible", null, new List<Type> { typeof(string), typeof(PropertyValue) }); | |
| new TypeValidator(new List<Type> | |
| { | |
| typeof(string), | |
| typeof(PropertyValue), | |
| typeof(Uri), | |
| typeof(string[]), | |
| typeof(PropertyValue[]), | |
| typeof(Uri[]), | |
| typeof(List<string>), | |
| typeof(List<PropertyValue>), | |
| typeof(List<Uri>), | |
| typeof(IList<string>), | |
| typeof(IList<PropertyValue>), | |
| typeof(IList<Uri>) | |
| }); |
| /// <summary> | ||
| /// The identifier property represents any kind of identifier for any kind of Thing, such as ISBNs, GTIN codes, UUIDs etc. | ||
| /// Schema.org: PropertyValue | Text | URL | ||
| /// </summary> | ||
| [JsonProperty("image")] | ||
| [JsonProperty("identifier", NullValueHandling = NullValueHandling.Ignore)] | ||
| public object Identifier | ||
| { | ||
| get { return identifier; } | ||
| set | ||
| { | ||
| IdentifierValidator.Validate(value); | ||
| identifier = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Adding Thing.Identifier introduces a duplicate identifier JSON property for types that already declare their own Identifier (e.g., Intangible/JobPosting.cs has [JsonProperty("identifier")] public object Identifier). Newtonsoft.Json typically treats duplicate JSON property names in an inheritance chain as an error or will serialize unpredictably. Consider removing the duplicate derived property (or making the base property virtual and overriding it) so each type only contributes one identifier member.
| get { return subjectOf; } | ||
| set | ||
| { | ||
| SubjectOfValidator.Validate(value); |
There was a problem hiding this comment.
SubjectOf is documented as CreativeWork | Event, but SubjectOfValidator only allows types from MXTires.Microdata.CreativeWorks (and CreativeWork). This will throw when a consumer sets SubjectOf to MXTires.Microdata.Event. Add typeof(Event) (or otherwise include Event) to the validator so it matches the documented/schema.org union.
| SubjectOfValidator.Validate(value); | |
| if (!(value is Event)) | |
| { | |
| SubjectOfValidator.Validate(value); | |
| } |
| } | ||
|
|
||
| //#endregion | ||
| return "<script type=\"application/ld+json\">" + json + "</script>"; |
There was a problem hiding this comment.
When wrapping JSON-LD in a <script> tag, the raw JSON is concatenated without escaping. If any string property contains </script> (or similar), it can break out of the tag and create an XSS vector in HTML contexts. Consider escaping the closing script sequence (e.g., replacing </ with <\/) when wrapInScriptTag is true, or returning the script tag via an HTML-encoding helper.
| return "<script type=\"application/ld+json\">" + json + "</script>"; | |
| var scriptSafeJson = json.Replace("</", "<\\/"); | |
| return "<script type=\"application/ld+json\">" + scriptSafeJson + "</script>"; |
| var validator = new TypeValidator( | ||
| "MXTires.Microdata.Intangible.StructuredValues.PriceSpecifications", | ||
| null, | ||
| new List<Type>() { typeof(PriceSpecification), typeof(IList<PriceSpecification>) }); |
There was a problem hiding this comment.
TypeValidator checks exact runtime types; allowing typeof(IList<PriceSpecification>) does not allow a List<PriceSpecification> instance. As written, assigning a List<PriceSpecification> to Offer.PriceSpecification will fail validation. Add typeof(List<PriceSpecification>) (and/or update the validator strategy to use assignability) so common list implementations are accepted.
| new List<Type>() { typeof(PriceSpecification), typeof(IList<PriceSpecification>) }); | |
| new List<Type>() { typeof(PriceSpecification), typeof(IList<PriceSpecification>), typeof(List<PriceSpecification>) }); |
| /// <summary> | ||
| /// Number or QuantitativeValue - The number of membership points earned by the member. | ||
| /// If necessary, the unitText can be used to express the units the points are issued in. (E.g. stars, miles, etc.) | ||
| /// </summary> | ||
| /// <value>The membership points earned.</value> | ||
| [JsonProperty("membershipPointsEarned")] | ||
| public QuantitativeValue MembershipPointsEarned { get; set; } | ||
|
|
There was a problem hiding this comment.
membershipPointsEarned is documented as Number or QuantitativeValue, but the property type is QuantitativeValue only. Elsewhere in the codebase, similar unions are modeled as object (e.g., FinancialProduct.AnnualPercentageRate). Consider changing this to object with type validation (number types + QuantitativeValue) so numeric values are supported.
| <TargetFrameworks>net461;net472;net48;net6.0;net8.0</TargetFrameworks> | ||
| <Version>1.0.4.6</Version> |
There was a problem hiding this comment.
TargetFrameworks no longer includes netcoreapp3.1, but the project file still contains an ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp3.1'" later in the file. This condition is now dead code and can be removed to avoid confusion and accidental maintenance drift.
This pull request introduces several enhancements and updates to the MXTires.Microdata library, focusing on extending the schema.org data model, improving property validation, modernizing project files, and updating dependencies. The most significant changes include the addition of new schema.org types and properties, refactoring for stricter property validation, and updates to project and test configurations for better compatibility and maintainability.
Schema.org Model Extensions:
MemberProgramTierinIntangible/MemberProgramTier.csto represent membership program tiers, including aTierRequirementproperty.PriceSpecificationwith new properties:EligibleQuantity,EligibleTransactionVolume,MembershipPointsEarned, andEligibleMemberTier, along with improved documentation and JSON serialization attributes. [1] [2]priceCurrencyproperty inPriceSpecificationwith appropriate JSON serialization.Property Validation and Serialization Improvements:
Thingclass to use staticTypeValidatorinstances for stricter and more maintainable property type validation on fields likeimage,mainEntityOfPage,identifier, andsubjectOf. Also, added support fordisambiguatingDescriptionand improved JSON serialization settings to ignore null values on most properties. [1] [2]PriceSpecificationproperty inOffer.csto use a more robustTypeValidatorconfiguration for type checking.Project and Dependency Updates:
Newtonsoft.Jsondependency from 13.0.3 to 13.0.4.Test Project Modernization:
.csprojformat, targeting .NET Framework 4.8, and switched to the latest MSTest NuGet packages.AssemblyInfo.csfile from the test project.Minor Cleanups:
usingdirectives fromProducts/Product.csfor code cleanliness.usingdirectives inPriceSpecification.csandThing.csto support new features and maintain consistency. [1] [2]These changes collectively improve the library's schema coverage, robustness, maintainability, and compatibility with modern .NET tooling.