Handle empty string fields in FieldConverter.

[jbreuer]

Description

This update addresses a specific issue encountered when dealing with JSON data for fields, such as the following example:

"fields": {
    "title": "",
    "leftLink": {
        "key": "40dfd6adb1334ff48c3e4c68ab9b9b35",
        "text": "The Energy Transition",
        "href": "/connections/energy-transition-and-sustainability/what-is-the-energy-transition"
    },
    "rightLink": {
        "key": "de61e12028784b4da96a9676b73fc764",
        "text": "There are currently 54 known outages.",
        "href": "/outages-and-maintenance"
    }
}

Previously, an empty string in the title field would trigger a JsonException in the FieldConverter because it was not recognized as a valid JsonValueKind type. This update introduces support for JsonValueKind.String, ensuring that empty strings are handled correctly alongside objects and arrays, thereby resolving the error.

Motivation

Custom Content Resolvers in both XM Cloud and Sitecore XP may return empty strings for certain fields, leading to errors in the FieldConverter. This enhancement ensures that such cases are handled properly, allowing for smooth operation regardless of whether an empty string is encountered.

Update: Following the discussion below, this PR has been revised to focus on adding an integration test for fields with empty strings instead of altering the FieldConverter.

[sc-ivanlieckens]

@jbreuer I have been contemplating this addition and am torn somewhat... When I consider what FieldConverter is, it's the supporting converter for IFieldReader and that is the support to read IField. This means that the result is expected to be an IField type which by definition makes this a class which doesn't really match with a primitive type such as "string" directly. The closest equivalent would be TextField but even that fieldtype has 2 properties: Value & EditableMarkup which means it should be an object type in the JSON...

Now as you stated a Custom Content Resolver can ignore the contract that exists, but by doing so it violates that contract, making it incompatible... In such case wouldn't it be sensible that the violator must add the matching Converter and this not be part of the SDK itself? Of course I'm also wondering: Is this possible? (it means needing to add this converter to the JsonSerializerOptions) Can we get to a good model of extensibility here or should we indeed be more loose with the contract?

@robearlam what do you think on the above and if we do not add this in the SDK would it make sense to add the OOTB custom resolvers that violate Fields being objects to the starterkit as Converters?

I'm also taking into account the fact that we are requesting people do not modify the CM instance in XMC and thus Custom Content Resolvers become extinct. In DXP they would indeed continue but how desirable it is to do API output shaping like this is another topic of debate...

[robearlam]

@jbreuer I have been contemplating this addition and am torn somewhat... When I consider what FieldConverter is, it's the supporting converter for IFieldReader and that is the support to read IField. This means that the result is expected to be an IField type which by definition makes this a class which doesn't really match with a primitive type such as "string" directly. The closest equivalent would be TextField but even that fieldtype has 2 properties: Value & EditableMarkup which means it should be an object type in the JSON...

Now as you stated a Custom Content Resolver can ignore the contract that exists, but by doing so it violates that contract, making it incompatible... In such case wouldn't it be sensible that the violator must add the matching Converter and this not be part of the SDK itself? Of course I'm also wondering: Is this possible? (it means needing to add this converter to the JsonSerializerOptions) Can we get to a good model of extensibility here or should we indeed be more loose with the contract?

@robearlam what do you think on the above and if we do not add this in the SDK would it make sense to add the OOTB custom resolvers that violate Fields being objects to the starterkit as Converters?

I'm also taking into account the fact that we are requesting people do not modify the CM instance in XMC and thus Custom Content Resolvers become extinct. In DXP they would indeed continue but how desirable it is to do API output shaping like this is another topic of debate...

This is also related to #8 . While we're moving away from customising the CM, it is still possible to change the structure of the returned JSON by changing to a different OOTB RCR as with the Navigation component mentioned in the issue above, or by writing your own IGQL query on the rendering item.

It feels like the json deserialisation we have here is too tight, as if the structure is changed then the whole deserialisation of the object blows up and the entire page wont render, meaning the only way to fix is to remove the offending component via the Content Editor, which is pretty horrible.

It would be nice if we included the existing structure mapping as default but made it simple to switch it for another custom implementation that customers could include themselves and patch in?

[jbreuer]

@jbreuer I have been contemplating this addition and am torn somewhat... When I consider what FieldConverter is, it's the supporting converter for IFieldReader and that is the support to read IField. This means that the result is expected to be an IField type which by definition makes this a class which doesn't really match with a primitive type such as "string" directly. The closest equivalent would be TextField but even that fieldtype has 2 properties: Value & EditableMarkup which means it should be an object type in the JSON...
Now as you stated a Custom Content Resolver can ignore the contract that exists, but by doing so it violates that contract, making it incompatible... In such case wouldn't it be sensible that the violator must add the matching Converter and this not be part of the SDK itself? Of course I'm also wondering: Is this possible? (it means needing to add this converter to the JsonSerializerOptions) Can we get to a good model of extensibility here or should we indeed be more loose with the contract?
@robearlam what do you think on the above and if we do not add this in the SDK would it make sense to add the OOTB custom resolvers that violate Fields being objects to the starterkit as Converters?
I'm also taking into account the fact that we are requesting people do not modify the CM instance in XMC and thus Custom Content Resolvers become extinct. In DXP they would indeed continue but how desirable it is to do API output shaping like this is another topic of debate...

This is also related to #8 . While we're moving away from customising the CM, it is still possible to change the structure of the returned JSON by changing to a different OOTB RCR as with the Navigation component mentioned in the issue above, or by writing your own IGQL query on the rendering item.

It feels like the json deserialisation we have here is too tight, as if the structure is changed then the whole deserialisation of the object blows up and the entire page wont render, meaning the only way to fix is to remove the offending component via the Content Editor, which is pretty horrible.

It would be nice if we included the existing structure mapping as default but made it simple to switch it for another custom implementation that customers could include themselves and patch in?

I agree with @robearlam's perspective. The current implementation enforces JsonValueKind to be Object or Array, causing exceptions that halt deserialization entirely. While my PR might not strictly return an IField type, it effectively prevents these exceptions, enabling continued access to the data for custom processing.

Allowing flexibility in deserialization would indeed be beneficial. Although supporting custom implementations should be easier, it should not be a strict requirement when using Custom Content Resolvers. My PR offers a pragmatic solution to avoid failures and maintain functionality.

[sc-ivanlieckens]

I'm not against flexibility in deserialization, I'm saying there's a more correct approach to this: Easily allow adding Custom Converters, rather than altering the existing Converter. And I believe all the capabilities for it are in place already, they simply need to be proofed out.

I don't want to violate the SRP of SOLID. The SRP of FieldConverter is to convert IField during deserialization.

[jbreuer]

I'm not against flexibility in deserialization, I'm saying there's a more correct approach to this: Easily allow adding Custom Converters, rather than altering the existing Converters. And I believe all the capabilities for it are in place already, they simply need to be proofed out.

I don't want to violate the SRP of SOLID. The SRP of FieldConverter is to convert IField during deserialization.

Easily allow adding Custom Converter sounds like a good idea. I could probably replace the FieldConverter with a custom converter that supports empty strings.

[robearlam]

I'm not against flexibility in deserialization, I'm saying there's a more correct approach to this: Easily allow adding Custom Converters, rather than altering the existing Converter. And I believe all the capabilities for it are in place already, they simply need to be proofed out.

I don't want to violate the SRP of SOLID. The SRP of FieldConverter is to convert IField during deserialization.

Completely agree here, we should be handling this via IoC - there's no point in having this implemented with Interfaces if we have the concrete classes performing multiple actions.

We would need to change how the middleware extensions are bound, currently the AddSystemTextJson is used to bind the different convertors. We should probably keep this method as it will work for most implementations, but then introduce an overload which allows developers to change individual convertors when necessary.

[jbreuer]

I'm not against flexibility in deserialization, I'm saying there's a more correct approach to this: Easily allow adding Custom Converters, rather than altering the existing Converter. And I believe all the capabilities for it are in place already, they simply need to be proofed out.
I don't want to violate the SRP of SOLID. The SRP of FieldConverter is to convert IField during deserialization.

Completely agree here, we should be handling this via IoC - there's no point in having this implemented with Interfaces if we have the concrete classes performing multiple actions.

We would need to change how the middleware extensions are bound, currently the AddSystemTextJson is used to bind the different convertors. We should probably keep this method as it will work for most implementations, but then introduce an overload which allows developers to change individual convertors when necessary.

An overload for the AddSystemTextJson method sounds like a great idea! I think that means this PR can be closed.

[sc-ivanlieckens]

@robearlam just FYI the AddSystemTextJson isn't really required and doesn't do anything since I had to split off the JsonSerializerOptions due to cache corruption. It does register the types and omg I just noticed what happened... I'll remove the method, it changes something it's not allowed to touch.

[sc-ivanlieckens]

@jbreuer can you create an integration test that simulates this issue?

[jbreuer]

@jbreuer can you create an integration test that simulates this issue?

Yes, I'll work on creating an integration test to simulate this issue. The test would likely need to handle a JSON structure similar to:

"fields": {
    "title": "",
    "leftLink": {
        "key": "40dfd6adb1334ff48c3e4c68ab9b9b35",
        "text": "The Energy Transition",
        "href": "/connections/energy-transition-and-sustainability/what-is-the-energy-transition"
    },
    "rightLink": {
        "key": "de61e12028784b4da96a9676b73fc764",
        "text": "There are currently 54 known outages.",
        "href": "/outages-and-maintenance"
    }
}

This should trigger the empty string handling issue. I'll update you when the test is ready!

[jbreuer]

@sc-ivanlieckens I’ve updated the headlessSxa.json to include a field with an empty string that also uses a Custom Content Resolver. The test passes, so everything seems to be working as expected now. I’m still unsure why it didn’t work when I tried it earlier.

This PR has now been updated to include the empty string in the integration test instead of adding JsonValueKind.String to the switch.

[sc-ivanlieckens]

@jbreuer make sure to consume the data as well. Usually that's where the error occurs. You can check #14 to see how I added Integration Tests for the Navigation

[jbreuer]

@jbreuer make sure to consume the data as well. Usually that's where the error occurs. You can check #14 to see how I added Integration Tests for the Navigation

@sc-ivanlieckens I’ve added the JSON with an empty string to the headlessSxa.json file. By running the test you introduced in PR #14, it now successfully deserializes into SitecoreLayoutResponseContent, even with the empty string. Everything seems to be working smoothly!

[sc-ivanlieckens]

Accepting this as adding the required test.