Opened 9 years ago

Closed 8 years ago

#708 closed defect (fixed)

Exceptions thrown on SELECTFEATURES instead of being handled gracefully.

Reported by: jng Owned by: brucedechant
Priority: high Milestone: 2.2
Component: Map Agent Version: 2.1.0
Severity: critical Keywords:
Cc: tomfukushima, brucedechant, stevedang External ID:

Description

When doing a SELECTFEATURES operation via the mapagent interface, exceptions are being thrown for trivial reasons instead of being handled gracefully by the mapagent.

Some examples using the Sheboygan data set.

1) Do a vanilla query on Library://Samples/Sheboygan/Data/Parcels.FeatureSource on class: Parcels with no filter

Invalid argument(s): [1] = "2" The geometry is invalid because it has too few coordinates.
Invalid argument(s): [1] = "2" The geometry is invalid because it has too few coordinates. Exception occurred in method MgLinearRing.MgLinearRing at line 37 in file d:\build\mapguide_open_source_v2.0\build_30.11\mgdev\common\geometry\LinearRing.cpp 

2) Do a vanilla query on Library://Samples/Symbolization/Highways/Data/Interstate.FeatureSource with class: interstate and no filter

Value for PREFIX property is null.
Value for PREFIX property is null. Exception occurred in method MgNullableProperty.CheckNull at line 73 in file d:\build\mapguide_open_source_v2.0\build_30.11\mgdev\common\foundation\Data/NullableProperty.cpp 

3) Do a vanilla query on Library://Samples/Sheboygan/Data/BuildingOutlines.FeatureSource with class: BuildingOutlines? and no filter

Argument is out of range.
Argument is out of range. Exception occurred in method MgByte.MgByte at line 34 in file d:\build\mapguide_open_source_v2.0\build_30.11\mgdev\common\foundation\Data/Byte.cpp 

Attachments (2)

MgProperty_XmlSerialization_NullValues.patch (8.9 KB) - added by jng 8 years ago.
Fix improper serialization of MgProperty? values
MgProperty_XmlSerialization_NullValues_V2.patch (11.3 KB) - added by jng 8 years ago.
Version 2 of the patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by jbirch

Version: 2.0.12.1.0

This is a pain in the ass, especially point 2. Null values aren't an error, they're a data modelling choice.

comment:2 Changed 8 years ago by jbirch

Cc: tomfukushima added

Changed 8 years ago by jng

Fix improper serialization of MgProperty? values

comment:3 Changed 8 years ago by tomfukushima

(Adding the comments that I sent to mapguide-internals to this ticket...)

I'm looking at the !MgProperty_XmlSerialization_NullValues.patch and it's close, but I think our schema is wrong. I believe these changes are for the schema defined by BatchPropertyCollection-1.0.0.xsd.

To be able to define null values, we only need to not specify the <Value> element. For example, to specify Owner=null we would write <Name>Owner</Name><Type>string</Type>. Compare with Owner="Tom" which is specified as <Name>Owner</Name><Type>string</Type><Value>Tom</Value>. Unfortunately our schema doesn't allow this because the Value element has the (default) constraints of minoccurs=1 and maxoccurs=1. I think that we should change the schema so that the value element is defined as:

<xs:element name="Value" type="xs:string" minOccurs="0"/>

Once we do this, then we can specify null values as above.

Strictly speaking, we would need to rev the schema, but I wonder if we would be able to get away with making the change in place this time (i.e., just updating the 1.0.0 schema). I guess, the problem would be if there are any clients that consume this format; they will need to be able to handle the case where the value elements are missing---however, given that no one has probably been able to use this because null values have caused crashes, I think no one would be affected. Not sure though, anyone see any problems with treating this as a bug in the schema and just fixing it?

Jackie's reply: If SELECTFEATURES is the only means through which this format can be consumed, then I think an in-place update is fine, seeing (as you already know) the operation hasn't worked properly in over 1.5 years.

Jason's reply: Is there JSON output for this? If so, how do you handle nulls for JSON output? I think there may be a native NULL type?

Jackie's reply: If there is, it isn't working:

http://trac.osgeo.org/mapguide/ticket/1104

But handling nulls would probably be the same, javascript can easily test for existence of object properties. So no "value" property -> null value.

Changed 8 years ago by jng

Version 2 of the patch

comment:4 Changed 8 years ago by jbirch

Just to keep this all together; it looks like JSON uses the keyword "null" (without quotes) to indicate null values.

http://www.json.org/

comment:5 Changed 8 years ago by tomfukushima

Cc: brucedechant stevedang added
Owner: set to brucedechant, stevedang

Patch looks good to me (I'm not sure though why we sometimes check the m_value as null---this is existing code, not new code added by Jackie). Bruce or Steve, could you review and submit the patch when you're good with it.

comment:6 Changed 8 years ago by tomfukushima

Adding info from Jackie's posting to mapguide internals: I've put up a 2nd version of the patch for review as another attachment for Ticket 708

This one omits the <Value> elements if the property value is null

This also patches the BatchPropertyCollection? xsd on the assumption that SELECTFEATURES (which is currently broken) is the sole provider of XML in this given schema.

...and from the above I would add... Reviewers: please ensure that BatchPropertyCollection?.xsd is the only schema that needs to be modified.

comment:7 Changed 8 years ago by tomfukushima

Bruce or Steve, will one of you be able to review this soon?

comment:8 Changed 8 years ago by brucedechant

Owner: changed from brucedechant, stevedang to brucedechant
Status: newassigned

I'll look at this next week.

comment:9 Changed 8 years ago by brucedechant

Milestone: 2.2
Resolution: fixed
Status: assignedclosed

I had to fix the patch before integrating it as it did not compile. It was using string and STRING with the + operator which does not work. I also had to replace the tabs with spaces.

Fixed. Trunk r4939

Note: See TracTickets for help on using tickets.