U4-4263 - U7 - HasValue method returns true for Json editors when there is no value

Created by Jeavon Leopold 19 Feb 2014, 10:00:10 Updated by Sebastiaan Janssen 19 Aug 2014, 13:27:08

This is because the property has an empty Json string in it "[]". I'm not sure if HasValue should be modified to return false if the value is only this or if we need a new method such as .HasJsonValue or similar?

Comments

Shannon Deminick 19 Feb 2014, 10:46:56

HasValue on what specifically?


Jeavon Leopold 19 Feb 2014, 10:56:34

For example related links picker: if (Model.Content.HasValue("relatedLinks")) workaround is to do something like: if (Model.Content.HasValue("relatedLinks") && Model.Content.GetPropertyValue("relatedLinks").Length > 2)


Jeavon Leopold 22 Apr 2014, 18:39:53

This also affects recursive requests, this should work, but if a item is cleared then it hits true as it contains "[]" var ancestorWithAWidget = Model.Content.GetPropertyValue<Dictionary<string, IEnumerable>>("widgetGrid", true); workaround in this case (Widget Grid) is tricky: var ancestorWithAWidget = Model.Content.AncestorsOrSelf().FirstOrDefault(x => x.GetPropertyValue<Dictionary<string, IEnumerable>>("widgetGrid") != null && x.GetPropertyValue<Dictionary<string, IEnumerable>>("widgetGrid").Count > 0);


Stephan 14 May 2014, 14:14:05

Unfortunately for backward compatibility we consider that a property has a value as soon as its serialized-to-string value is not an empty string. Anything more complex would require help from the IPropertyValueConverter (ie it should implement the .IsValue(PublishedPropertyType propertyType, object source) method). I think this is the proper way to do it (as a numeric zero could mean "no value" for some editors) but this would be a breaking change of the IPublishedValueConverter interface.

Glad to do it once we are allowed breaking changes.

Alt. in the meantime we ''could'' try to detect whether the property is Json... how? Fine if it uses the internal Json converter, otherwise...?

Alt. couldn't we save an empty string instead of "[]" when there's no value?


Jeavon Leopold 14 May 2014, 14:37:44

Makes sense, so I think for the core JSON saving editors, Related Links and Image Cropper this could be fixed by means of a value converter?

For other property editors, the value converter needs to return null if the JSON is empty, right?

I guess this means that HasValue and recursive requests are just not going to work for pure manifest/js/html editors, they will need a value converter....?


Stephan 14 May 2014, 14:45:37

Would be fixed by means of an "adapted" value converter with the new IsValue() method and then HasValue & recursive would work. But we can't have such value converter right now (breaking change). Best change I think is at ''property editor'' level at the moment, to ensure that an "empty json" is stored as NULL not "[]". Making sense?


Jeavon Leopold 14 May 2014, 15:29:39

Yes, it makes sense. I have seen such a method implemented as below:

$scope.$on("formSubmitting", function(ev, args) { if( $scope.renderModel.length ) { $scope.model.value = $scope.renderModel } else { $scope.model.value = null } })


Shannon Deminick 14 May 2014, 23:16:19

Probably better to do this sort of thing in the c# property editor, will look into that and should be easy to do. If the json is [] or then we can return null.


Nicholas Westby 06 Aug 2014, 16:10:25

As a workaround for a similar issue, what I do is create a list of known empty values and check against that array (rather than just checking for empty strings): https://github.com/rhythmagency/rhythm.umbraco.extensions/blob/master/trunk/Rhythm.Extensions/Rhythm.Extensions/ExtensionMethods/PublishedContentExtensionMethods.cs#L643


Shannon Deminick 15 Aug 2014, 19:03:15

@zpqrtbnk After looking into this, there are a couple of ways to achieve this and perhaps we implement both:

  • When a value is saved and it's a json property type, we'll check if it is an empty json object/array and then return null - this means that the value is null in the database and in the cache and therefore the HasValue will work as-is.
  • Currently in the IPublishedProperty.HasValue implementations, it is implementation specific, meaning the xml cache looks at the xml string, the db cache looks at the string stored in the db, etc... BUT, wouldn't it be better if all of these implementations retrieved the _objectValue.Value to check if that value is null or whitespace? That way the value that is checked is output by the value converters? Or is it currently like this for performance reasons?

I'll implement the first fix since that is pretty easy and makes sense, just means that this issue won't be fixed for people that already have data, they'll have to re-save their documents with these properties.


Shannon Deminick 15 Aug 2014, 19:17:17

I fixed the first part in rev: 69244f6991aee45e68b802afa5d106063251e250 if you want to have a look


Jeavon Leopold 19 Aug 2014, 13:23:45

Works perfectly, thanks!


Sebastiaan Janssen 19 Aug 2014, 13:26:27

Set to "Fixed" a bit too quickly, there's a second part that's not fixed yet ;)


Priority: Normal

Type: Bug

State: Fixed

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.0.1, 7.0.2, 7.0.3

Due in version: 7.1.5

Sprint:

Story Points:

Cycle: