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

Bug fixes #23

Merged
merged 3 commits into from
Apr 27, 2018
Merged

Bug fixes #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43afd7d
AddEntity shouldn't cache by default unless link is used.
Apr 9, 2018
9e37069
Remove the requirement to provide class in FindEntity
Apr 9, 2018
9f385f8
Return "missing property" when getting results as list
Apr 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/PMGDQueryHandler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,10 +484,12 @@ void PMGDQueryHandler::build_results(Iterator &ni,
for (; ni; ni.next()) {
for (int i = 0; i < keyids.size(); ++i) {
Property j_p;
if (!ni->check_property(keyids[i], j_p))
continue;
PMGDPropList &list = rmap[qn.response_keys(i)];
PMGDProp *p_p = list.add_values();
if (!ni->check_property(keyids[i], j_p)) {
construct_missing_property(p_p);
continue;
}
construct_protobuf_property(j_p, p_p);
}
count++;
Expand Down Expand Up @@ -584,3 +586,10 @@ void PMGDQueryHandler::construct_protobuf_property(const Property &j_p, PMGDProp
p_p->set_blob_value(j_p.blob_value().value, j_p.blob_value().size);
}
}

void PMGDQueryHandler::construct_missing_property(PMGDProp *p_p)
{
// Assumes matching enum values!
p_p->set_type(PMGDProp::StringType);
p_p->set_string_value("Missing property");
}
1 change: 1 addition & 0 deletions src/PMGDQueryHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ namespace VDMS {
const PMGDQueryNode &qn,
PMGDCmdResponse *response);
void construct_protobuf_property(const PMGD::Property &j_p, PMGDProp*p_p);
void construct_missing_property(PMGDProp *p_p);

void set_response(PMGDCmdResponse *response, PMGDCmdErrorCode error_code,
std::string error_msg)
Expand Down
5 changes: 3 additions & 2 deletions src/RSCommand.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,10 @@ int AddEntity::construct_protobuf(PMGDQuery& query,
Json::Value& error)
{
const Json::Value& cmd = jsoncmd[_cmd_name];
bool link = cmd.isMember("link");

int node_ref = get_value<int>(cmd, "_ref",
query.get_available_reference());
link ? query.get_available_reference() : -1);

query.AddNode(
node_ref,
Expand All @@ -165,7 +166,7 @@ int AddEntity::construct_protobuf(PMGDQuery& query,
cmd["constraints"]
);

if (cmd.isMember("link")) {
if (link) {
add_link(query, cmd["link"], node_ref, VDMS_GENERIC_LINK);
}

Expand Down
2 changes: 1 addition & 1 deletion tests/AddAndFind_query.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@
"Age": [">", 0, "<=", 100 ]
},
"results": {
"list":["Name","Age","Email"],
"list":["Name","Age","Email", "Study"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this is not checking whether the response is correct.
We should add a tests that checks whether we are getting the right response.

I think we should move this tests to python, instead of using this file and C++ (not for this, but for another different pull request for later)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I did my usual thing of visual inspection 😀

"sort" :"Age"
}
}
Expand Down
10 changes: 9 additions & 1 deletion tests/json_queries.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,15 @@ TEST(QueryHandler, AddAndFind){
if (cmd =="FindEntity")
out_query_num++;

if (j == 12) { // Last FindEntiyu
if (j == 11) { // Second Last FindEntity
EXPECT_EQ(query["FindEntity"]["entities"][2]["Study"].asString(),
"Missing property");

EXPECT_EQ(query["FindEntity"]["entities"][3]["Study"].asString(),
"Missing property");
}

if (j == 12) { // Last FindEntiy
EXPECT_EQ(query["FindEntity"]["entities"][0]["Birthday"].asString(),
"1946-10-07T17:59:24-07:00");

Expand Down
2 changes: 0 additions & 2 deletions utils/src/api_schema/api_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,6 @@
"results": { "type": "object" },
"unique": { "type": "boolean" }
},

"required": ["class"],
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing the "class" requirement?
This requirement is specified by the API docs.

If we don't require, we need to make sure we are handling blobs/images/descriptors/videos well as well, when, say, the results block has the "return_blob" flag.

We should probably think more about this before removing the requirement, as it introduces a lot of undefined behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how those are correlated. class is what we have for the tag in PMGD. It should not affect for blobs. You could give a class for an entity that has no blob and still ask for return blob. For images, descriptors etc, it is counter intuitive to expect class since those are implicit anyway. What am i missing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

so, If we do findEntity with no class, we need to filter out those entities that are images/descriptors/videos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Aren't they regular nodes with properties in the graph? It does affect dealing with blob some cause technically you have blobs associated with them and if someone asked for a blob, I am not sure the corresponding property names would match.

Copy link
Contributor

Choose a reason for hiding this comment

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

and if that is the case, we need to fix findEntity to do that.

I would leave it required until we have the necessary mechanism to deal with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine doing it. It is an unnecessary limitation. Besides, blob is the one not checked in yet and so it is possible to handle it. This is already in our repo. But it is a useful capability to be able to search across all nodes without class barrier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this leaves us a more complicated way of enumerating all entities. Another reason to remove the restriction. And an unrelated note: we should probably have some way of returning the class of an entity

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. You convinced me. Makes sense.

I will update the wiki page and remove the requirement.

"additionalProperties": false
},

Expand Down