Skip to content

For NGSI-v2, ensure GeoJSON is correctly encode in the request. #854

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.

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

Merged
merged 6 commits into from
Dec 18, 2020

Conversation

jason-fox
Copy link
Contributor

I've noticed that the NGSI-v2 entity processor does not encode geo:json properly - this PR alters the attribute from a string to a geo:json as shown:

 {
   "id": "TheFirstLight",
   "type": "TheLightType",
   "location": {
-    "type": "geo:point",
-    "value": "0, 0"    
+    "type": "geo:json",
+    "value": {
+        "coordinates": [
+            0,
+            0
+        ],
+        "type": "Point"
+    }   
   }
 }

This work has already been done for GeoProperties for NGSI-LD, and it is merely a matter of back-porting to v2 and ensuring consistency between the provisioning of both formats.

This PR does the following:

  • Move Lat/Lng processing to a common location
  • For NGIS-v2, iterate attributes and convert common GeoJSON types
  • Ensure that if a GeoJSON object is parsed, it remains as GeoJSON (v2 and LD)
  • Amend test expectations to ensure geo:json is properly formated.
  • Add new NGSI-v2 tests for GeoJSON
  • Update documentation

It is small, so I assume it makes sense to merge into the ongoing NGSI-LD PR, but it could also be pushed to master after feature/842_ngsi_ld has landed.

@fgalan
Copy link
Member

fgalan commented Apr 6, 2020

I've noticed that the NGSI-v2 entity processor does not encode geo:json properly - this PR alters the attribute from a string to a geo:json as shown:

 {
   "id": "TheFirstLight",
   "type": "TheLightType",
   "location": {
-    "type": "geo:point",
-    "value": "0, 0"    
+    "type": "geo:json",
+    "value": {
+        "coordinates": [
+            0,
+            0
+        ],
+        "type": "Point"
+    }   
   }
 }

Not sure of unerstanding this PR... "geo:point" attribute type (using as value a string with comma separate values) is a legal value en NGSIv2.

The provision of the IOTA devices should specify the type used in the CB update. I mean:

  • If geo:point is used as attribute type in the device provision, then a string with comma separate coordinates is used as attribute value
  • If geo:json is used as attribute type in the device provision, then a valid GeoJSON object is used as attribute value

Is this the way it works after the PR? In that case, which particular aspect has fixed the PR?

@jason-fox
Copy link
Contributor Author

jason-fox commented Apr 6, 2020

Not sure of understanding this PR... "geo:point" attribute type (using as value a string with comma separated values) is a legal value en NGSIv2.

It is legal NGSIv2 but meaningless - an attribute defined as: "type": "geo:point", "value": "0, 0" or "type": "geo:json", "value": "0, 0" or geo:whatever currently has no special processing and is basically just a string, and it shouldn't be - it should be GeoJSON.

However, the type geo:json is defined with a special meaning and format in NGSIv2, and this is what the PR fixes. The example could have been written like this:

 {
   "id": "TheFirstLight",
   "type": "TheLightType",
   "location": {
-    "type": "geo:json",
-    "value": "0, 0"    
+    "type": "geo:json",
+    "value": {
+        "coordinates": [
+            0,
+            0
+        ],
+        "type": "Point"
+    }   
   }
 }

Only the updated version is a valid location GeoProperty in NGSIv2 and can be accessed using geo-queries. "type": "geo:json", "value": "0, 0" is invalid NGSIv2.

In addition, for equivalence with the NGSI-LD changes, the following types are also converted to proper GeoJSON types

  • geo:json, geo:point, Point, GeoProperty - Point
  • geo:linestring, LineString - LineString
  • geo:polygon, Polygon - Polygon
  • geo:multipoint', MultiPoint - MultiPoint
  • geo:multilinestring, MultiLineString - MultiLineString

https://github.com/jason-fox/iotagent-node-lib/blob/feature/ngsi-ld/lib/services/ngsi/entities-NGSI-v2.js#L50-L85

Note that GeoProperty, Point, LineString, Polygon, MultiPoint and MultiLineString are all reserved words in the NGSI-LD core context, so you cannot legally set the values to comma separated strings.

@fgalan
Copy link
Member

fgalan commented Jun 9, 2020

So the fragment in the PR body

 {
   "id": "TheFirstLight",
   "type": "TheLightType",
   "location": {
-    "type": "geo:point",
-    "value": "0, 0"    
+    "type": "geo:json",
+    "value": {
+        "coordinates": [
+            0,
+            0
+        ],
+        "type": "Point"
+    }   
   }
 }

has a typo and really it is

 {
   "id": "TheFirstLight",
   "type": "TheLightType",
   "location": {
-    "type": "geo:json",
-    "value": "0, 0"    
+    "type": "geo:json",
+    "value": {
+        "coordinates": [
+            0,
+            0
+        ],
+        "type": "Point"
+    }   
   }
 }

Is my understanding correct, pls?

@fgalan
Copy link
Member

fgalan commented Jun 9, 2020

In addition, with regards, to:

In addition, for equivalence with the NGSI-LD changes, the following types are also converted to > proper GeoJSON types

  • geo:json, geo:point, Point, GeoProperty - Point
  • geo:linestring, LineString - LineString
  • geo:polygon, Polygon - Polygon
  • geo:multipoint, MultiPoint - MultiPoint
  • geo:multilinestring, MultiLineString - MultiLineString

Question: geo:linestring, geo:multipoint' and geo:multilinestring comes from the NGSI-LD specification?

@jason-fox
Copy link
Contributor Author

jason-fox commented Jun 9, 2020

Currently geo:json, geo:point, Point and GeoProperty are all treated as the GeoJSON Point type.

1)geo:json is specified in the NGSI v2 spec.
2) Point is defined by rfc7946
3) geo:point has been added as a synonym for consistency - internally this is just a switch statement after all.
4) GeoProperty is defined in the NGSI-LD spec

Within the NGSI-LD IoT Agent, the default functionality is to treat each attribute as a property and the value as the native JSON type. but NGSI-LD treats GeoJSON type as native as well. Therefore
GeoProperty has to map to some GeoJSON type- Point is the obvious default.

Yes there was a typo in the intial diff example. The type doesn't change in NGSI-v2, only the value does.

NGSI v2

The idea for NGSI-v2 is to provision the attribute with geo:json or geo:point and for the value to be a valid GeoJSON Point. So a GPS device for example is free to pass in an array of numbers or a string of numbers and the result is valid NGSi-v2 geo:json type..

NGSI LD

The equivalent for NGSI-LD is to provision the attribute with Point or GeoProperty and for the value to be a valid GeoJSON Point. So a GPS device for example is free to pass in an array of numbers or a string of numbers and the result is valid GeoProperty.

@jason-fox
Copy link
Contributor Author

geo:point, geo:linestring, geo:multipoint and geo:multilinestring are merely synonyms, they are not defined in rfc7946 (Point, LineString, MultiPoint and MultiLineString are though)

@jason-fox
Copy link
Contributor Author

geo:point had been (Mis?)used in one of the existing NGSI v2 tests:

test/unit/ngsiv2/examples/contextRequests/updateContextStaticAttributes.json

So I assumed that it was supposed to be a synonym for geo:json (which is well defined in NGSI-v2)

    -  Move Lat/Lng processing to a common location
    -  For NGIS-v2, iterate attributes and convert common GeoJSON types
    -  Ensure that if a GeoJSON object is parsed, it remains as GeoJSON (v2 and LD)
    -  Amend test expectations to ensure geo:json is properly formated.
    -  Add new NGSI-v2 tests for GeoJSON

-  Basic GeoJSON documentation.

-  Ensure consistent GeoProvisioning …

    - NGSI-LD supports geo:json as alias for GeoProperty
    - Add expression tests
    - Amend expectations.
@manucarrace manucarrace deleted the branch telefonicaid:master November 26, 2020 14:57
@fgalan fgalan reopened this Nov 26, 2020
@fgalan fgalan changed the base branch from feature/842_ngsi_ld to master November 26, 2020 15:11
@fgalan
Copy link
Member

fgalan commented Nov 26, 2020

Once PR #849 the old base brach feature/842_ngsi_ld disappears. Base branch of this PR changed to master.

@jason-fox
Copy link
Contributor Author

This PR is ready for Review, but I'd suggest actioning #943 first so the builds aren't queuing on Travis for hours before each suggested change.

@fgalan
Copy link
Member

fgalan commented Nov 26, 2020

This PR is ready for Review, but I'd suggest actioning #943 first so the builds aren't queuing on Travis for hours before each suggested change.

For some weeks now, I have observed the long queuing delay you mention. Time ago, the queuing delay was not so high. Do you know if it is due to some change in Travis policy, usage terms or so?

(Just curiosity...)

@jason-fox
Copy link
Contributor Author

jason-fox commented Nov 27, 2020

Back in October Travis started throttling FOSS builds to a maximum of 500 concurrent Linux Machines across their infrastructure, this causes a backlog of unfulfilled builds to occur:

Screenshot 2020-10-27 at 18 30 37

Since then, as you can see the backlog now spikes during the North American Workday. This is to encourage the transition to travis's commercial product travis-ci.com

@fgalan
Copy link
Member

fgalan commented Nov 27, 2020

This PR is ready for Review, but I'd suggest actioning #943 first so the builds aren't queuing on Travis for hours before each suggested change.

Done. Please sync the PR with master to get the changes.

@jason-fox
Copy link
Contributor Author

Please sync the PR with master to get the changes.

Done.

@@ -46,6 +46,64 @@ curl http://${KEYSTONE_HOST}/v3/OS-TRUST/trusts \
Apart from the generation of the trust, the use of secured Context Brokers should be transparent to the user of the IoT
Copy link
Member

Choose a reason for hiding this comment

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

Should this PR include a CHANGES_NEXT_RELEASE entry? If I understand correctly the discussion on #961, this PR would be fixing that issue, so it could be mentioned in the CHANGES_NEXT_RELEASE entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed 51b8f10 + f4ebb69 - CNR updated

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit a7eb260 into telefonicaid:master Dec 18, 2020
jason-fox added a commit to jason-fox/iotagent-ul that referenced this pull request Jan 4, 2021
@jason-fox jason-fox deleted the feature/ngsi-ld branch February 5, 2021 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants