Skip to content
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

Feature/1677 geo location #1801

Merged
merged 19 commits into from
Feb 17, 2016
Merged

Feature/1677 geo location #1801

merged 19 commits into from
Feb 17, 2016

Conversation

kzangeli
Copy link
Member

Added some stuff missing for geo-location in URI params #1677

functest running ...

@kzangeli kzangeli added this to the 0.28.0 milestone Feb 16, 2016
@@ -243,7 +243,7 @@ static int uriArgumentGet(void* cbDataP, MHD_ValueKind kind, const char* ckey, c
{
containsForbiddenChars = forbiddenChars(val, ";");
}
else if ((key != "q") && (key != "idPattern"))
else if ((key != "q") && (key != "idPattern") && (key != "geometry") && (key != "georel"))
Copy link
Member

Choose a reason for hiding this comment

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

Note that geometry has its own "if clause" some lines above (around 238).

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, this was already taken care of.
Didn't see that ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 6bcd211 (georel treated as geometry)

@kzangeli
Copy link
Member Author

Finally got some time to run functests on this branch ...
Had a few failures, now fixed (01ec8be)

Complete 'make test' running (again) right now

@@ -299,7 +299,7 @@ Fields used as identifiers in the NGSIv2 API follow special rules regarding allo

The rules are:

* Allowed characters are the ones in the plain ASCII set except the following ones: whitespace, `&`, ?`, `/` and `#`.
* Allowed characters are the ones in the plain ASCII set except the following ones: whitespace, `&`, `?`, `/` and `#`.
Copy link
Member

Choose a reason for hiding this comment

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

Not actually related with this PR, but a small typo known time ago now being fixed. NTC

@@ -20,3 +20,5 @@
- Add: detect forbidden chars in entity ids and attr names in URI (Issue #1793)
- Add: dates support in attribute values (Issue #1038)
- Add: implemented new operation: POST /v2/op/query (Issue #1080)
- Fix: NGSIv2 URI param 'georel' (along with 'geometry' and 'coords') proper support (Issue #1677)
- Add: scope FIWARE::Location::NGSIv2 to allow using NGSIv2 geo-queries also with NGSIv1 (Issue #1677)
Copy link
Member

Choose a reason for hiding this comment

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

This last line is not actually part of this PR, but soon to be added.

NTC

else if (scoP->areaType == orion::BoxType)
{
BSONArrayBuilder ps;
ps.append(BSON_ARRAY(scoP->box.lowerLeft.longitude() << scoP->box.lowerLeft.latitude()));
Copy link
Member

Choose a reason for hiding this comment

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

In order this to work, lowerLeft has to be the real lower left corner and upperRight has to be the real upper right corner. Is the fill() method at Scope class ensuring this?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed over skype - no, there is no check/fix for that right now. Must be implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 92aaa61

@fgalan
Copy link
Member

fgalan commented Feb 16, 2016

Ready for review!

@kzangeli
@crbrox

{
type = FIWARE_LOCATION_V2;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I wrote these 8 lines ..

Now I wish I had done it like this, on one line:

type = (apiVersion == "v1")? FIWARE_LOCATION : FIWARE_LOCATION_V2:

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 92aaa61

@kzangeli
Copy link
Member Author

I just realized a thing I've forgotten to implement - the release of the area objects.
Look at the method Scope::release():

/* ****************************************************************************
*
* release -
*/
void Scope::release(void)
{
  polygon.release();
}

We need to call the release() methods also for point, line, and box (and circle?), and create the methods if they don't exist.

@@ -281,7 +309,31 @@ int Scope::fill
pointV.clear();
return -1;
}
box.fill(pointV[0], pointV[1]);

// Check that points are different an not aligned (either horizontally or vertically)
Copy link
Member Author

Choose a reason for hiding this comment

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

type: an => and

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 367b713

@kzangeli
Copy link
Member Author

LGTM

1 similar comment
@crbrox
Copy link
Member

crbrox commented Feb 17, 2016

LGTM

crbrox added a commit that referenced this pull request Feb 17, 2016
@crbrox crbrox merged commit ae86f89 into develop Feb 17, 2016
@crbrox crbrox deleted the feature/1677_geo_location branch February 17, 2016 12:41
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