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.

Sign up for GitHub

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

Jump to bottom

Hardening/1298 render compound simplification #3292

Merged
merged 5 commits into from
Sep 14, 2018
Merged

Hardening/1298 render compound simplification #3292

merged 5 commits into from
Sep 14, 2018

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Sep 12, 2018

This PR continues the render unification and simplification work in issue #1298. In particular:

  • Domain attribute and domain metadata stuff has been removed (never implemented in NGSIv1)
  • CompoundValueNode::render() removed. It is not actually needed, as the JSON generated by toJson() is exactly the same. A good side-effect of this is that apiVersion doesn't need to be passed down in the render method functions, which help to decouple a bit more NGSIv1 render code and NGSIv2 render code.

Some .test has been modified, but I think they are needed modifications and doesn't break backward compability. Please find specific comment on the .test files.

@fgalan fgalan mentioned this pull request Sep 12, 2018
Open
fgalan
fgalan commented Sep 12, 2018
View reviewed changes
test/functionalTest/cases/0000_json_parse/jsonParseGetNotifyContext.test
@@ -39,7 +39,6 @@ payload='{
"type": "Room",
"isPattern": "false",
"id": "ConferenceRoom",
"attributeDomainName" : "ADN",
Copy link
Member Author

Choose a reason for hiding this comment

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

Obviously, attributeDomainName and metadata domain has been removed from .test.

Copy link
Member Author

Choose a reason for hiding this comment

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

NTC

fgalan
fgalan commented Sep 12, 2018
View reviewed changes
test/functionalTest/cases/1156_mq_as_uri_param_for_metadata/mq_as_scope_in_v1_queries.test
@@ -183,7 +183,7 @@ Date: REGEX(.*)
05. GET entities matching q=A1==1 AND mq=A1.M1==2, see E2, using /v1/queryContext
=================================================================================
HTTP/1.1 200 OK
Content-Length: 246
Content-Length: 245
Copy link
Member Author

Choose a reason for hiding this comment

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

This is due to a "\n" has been removed in NGSIv1 rendering. That "\n" was probably forgotten when the de-indent work in NGSIv1 was done.

Copy link
Member Author

Choose a reason for hiding this comment

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

NTC

fgalan
fgalan commented Sep 12, 2018
View reviewed changes
test/functionalTest/cases/1156_qfilters_and_compounds/qfilters_and_compounds_equals_null.test
@@ -86,7 +86,7 @@ echo
===================================================================
HTTP/1.1 201 Created
Content-Length: 0
Location: /v2/entities/E1?type=none
Location: /v2/entities/E1?type=Thing
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking to the version of this .test in master is seems this PR has solved a bug in default typing (although I'm not sure how I have done that!:)

Probably this deserves a line in CNR, something like:

- Fix: default types for entities and attributes in NGSIv2 was wrongly using "none" in some cases

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 6fb9656

fgalan
fgalan commented Sep 12, 2018
View reviewed changes
test/functionalTest/cases/2339_json_native_types_in_v1/json_native_types_in_v1.test
@@ -155,7 +155,7 @@ Date: REGEX(.*)
02. GET the entity using /v1/queryContext
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in Content-Length in this .test due to "\n" removal in rendering logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

NTC

fgalan
fgalan commented Sep 12, 2018
View reviewed changes
test/functionalTest/cases/2507_csub_metadata_field/previous_value_with_compounds_ngsiv1.test
@@ -229,7 +229,7 @@ Date: REGEX(.*)
========================================================================
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in Content-Length in this .test due to "\n" removal in rendering logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

NTC

fgalan
fgalan commented Sep 12, 2018
View reviewed changes
.../functionalTest/cases/2507_csub_metadata_field/special_metadata_in_notifications_ngsiv1.test
@@ -356,7 +356,7 @@ Date: REGEX(.*)
=====================================================================================
POST http://localhost:REGEX(\d+)/notify
Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in Content-Length in this .test due to "\n" removal in rendering logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

NTC

fgalan
fgalan commented Sep 12, 2018
View reviewed changes
src/lib/ngsi/Metadata.cpp
// So, if V2 rendering is destroyed by this modification, it is only because the V2 rendering is using
// a method that it SHOULD NOT USE !
//
out += "\n";
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the "\n" that I mention in .test comment was removed.

NTC (informative)

kzangeli
kzangeli approved these changes Sep 13, 2018
View reviewed changes
Copy link
Member

@kzangeli kzangeli left a comment

Choose a reason for hiding this comment

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

LGTM

src/lib/ngsi/ContextElement.cpp Outdated
bool commaAfterContextAttributeVector = domainMetadataVectorRendered;
bool commaAfterAttributeDomainName = domainMetadataVectorRendered || contextAttributeVectorRendered;
bool commaAfterEntityId = commaAfterAttributeDomainName || attributeDomainNameRendered;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

All this to be removed in this PR, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I missed it...

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 6ddd2cb

kzangeli
kzangeli approved these changes Sep 13, 2018
View reviewed changes
Copy link
Member

@kzangeli kzangeli left a comment

Choose a reason for hiding this comment

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

LGTM

@AlvaroVega AlvaroVega merged commit e95d052 into master Sep 14, 2018
@AlvaroVega AlvaroVega deleted the hardening/1298_render_compound_simplification branch September 14, 2018 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlvaroVega AlvaroVega AlvaroVega approved these changes

@kzangeli kzangeli kzangeli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fgalan @AlvaroVega @kzangeli