You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/07/01 16:20:42 UTC
[GitHub] [qpid-proton] kpvdr opened a new pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
kpvdr opened a new pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263
All tests pass.
It appears necessary to test for both pn_is_in_array() && pn_is_first_in_array(), using only the latter causes failures in the list tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] kpvdr commented on pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
kpvdr commented on pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#issuecomment-656768640
@gemmellr: The question of testing against the expected encoding on a byte-for-byte basis should affect ALL codec tests, not just the array tests. Do we check this for other AMQP type encodings? That being said, there is nothing wrong with introducing this for array tests. I agree with your analysis that it would pick up additional test and encoding path issues that just comparing the inspect() values would not do.
Agreed on adding additional list test cases as you have described, this is fortunately trivial.
With the proposed fix for the array empty list issue, I think all lists, even when all lists are empty, will result in the long (4-byte) encoding for each empty list. I think checking each and every array element for emptiness so that the empty list encoding could be used would probably add noticeable overhead (not to mention some additional complexity) to the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] kpvdr commented on a change in pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#discussion_r448993947
##########
File path: c/src/core/encoder.c
##########
@@ -336,8 +336,10 @@ static int pni_encoder_exit(void *ctx, pn_data_t *data, pni_node_t *node)
pn_encoder_t *encoder = (pn_encoder_t *) ctx;
char *pos;
- // Special case 0 length list
- if (node->atom.type==PN_LIST && node->children-encoder->null_count==0) {
+ // Special case 0 length list, but not as first element in an array
+ pni_node_t *parent = pn_data_node(data, node->parent);
+ if (node->atom.type==PN_LIST && node->children-encoder->null_count==0 &&
+ !(pn_is_in_array(data, parent, node) && pn_is_first_in_array(data, parent, node))) {
Review comment:
The type opcode for array elements is encoded only once, and is determined by the type of the first element (hence the original check). However, the size/count fields for each element (even empty lists) must also be present as the data portion of each array element, and it is these that are omitted for all empty lists if this patch update is not made. Thanks for spotting that!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] kpvdr commented on a change in pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#discussion_r448993947
##########
File path: c/src/core/encoder.c
##########
@@ -336,8 +336,10 @@ static int pni_encoder_exit(void *ctx, pn_data_t *data, pni_node_t *node)
pn_encoder_t *encoder = (pn_encoder_t *) ctx;
char *pos;
- // Special case 0 length list
- if (node->atom.type==PN_LIST && node->children-encoder->null_count==0) {
+ // Special case 0 length list, but not as first element in an array
+ pni_node_t *parent = pn_data_node(data, node->parent);
+ if (node->atom.type==PN_LIST && node->children-encoder->null_count==0 &&
+ !(pn_is_in_array(data, parent, node) && pn_is_first_in_array(data, parent, node))) {
Review comment:
The type opcode for array elements is encoded only once, and is determined by the type of the first element (hence the original check). However, the size/count fields for each list element (even empty lists) must also be present as the data portion of each array element, and it is these that are omitted for any empty lists following the first if this patch update is not made. Thanks for spotting that!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] kpvdr commented on a change in pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#discussion_r448993947
##########
File path: c/src/core/encoder.c
##########
@@ -336,8 +336,10 @@ static int pni_encoder_exit(void *ctx, pn_data_t *data, pni_node_t *node)
pn_encoder_t *encoder = (pn_encoder_t *) ctx;
char *pos;
- // Special case 0 length list
- if (node->atom.type==PN_LIST && node->children-encoder->null_count==0) {
+ // Special case 0 length list, but not as first element in an array
+ pni_node_t *parent = pn_data_node(data, node->parent);
+ if (node->atom.type==PN_LIST && node->children-encoder->null_count==0 &&
+ !(pn_is_in_array(data, parent, node) && pn_is_first_in_array(data, parent, node))) {
Review comment:
The type opcode for array elements is only once, and is determined by the type of the first element (hence the original check). However, the size/count fields for each element (even empty lists) must also be present as the data portion of each array element, and it is these that are omitted for all empty lists if this patch update is not made. Thanks for spotting that!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
astitcher commented on pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#issuecomment-654321007
@kpvdr Thanks for adding regression tests for this bug.
We're trying to avoid adding C unit tests in python with a long term aim of avoiding python for the C unit tests completely. So it'd be great to put them in the existing unit test file c/tests/data_test.cpp.
You can use C++ in these unit tests (as we are already using a C++ unit test framework) which allows much of the same convenience as writing the tests in python.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] kpvdr commented on a change in pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#discussion_r448980074
##########
File path: c/src/core/encoder.c
##########
@@ -336,8 +336,10 @@ static int pni_encoder_exit(void *ctx, pn_data_t *data, pni_node_t *node)
pn_encoder_t *encoder = (pn_encoder_t *) ctx;
char *pos;
- // Special case 0 length list
- if (node->atom.type==PN_LIST && node->children-encoder->null_count==0) {
+ // Special case 0 length list, but not as first element in an array
+ pni_node_t *parent = pn_data_node(data, node->parent);
+ if (node->atom.type==PN_LIST && node->children-encoder->null_count==0 &&
+ !(pn_is_in_array(data, parent, node) && pn_is_first_in_array(data, parent, node))) {
Review comment:
Agreed. You are correct, this needs to happen for ALL list array members that are empty. I have checked in an update and tested it with empty lists as several members of the array including first, and all now work correctly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] gemmellr commented on pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#issuecomment-656728966
I would be inclined to add some tests that verify the encoded bytes are exactly as expected, and not just encode+decode things to check that the pn_data that goes into encode is what comes out of decode. The latter is still useful, but it mainly verifies there isnt a different understanding between the encoder and decoder, and not really that they actually encoded/decoded things correctly.
Indeed, as you have pointed out the formatting stuff being used in the test is actually currently encoding String/Symbol values differently that its described behaviour (which as Andrew noted elsewhere may just be a internal doc issue, but every single use of those values now needs to be checked to ensure that). The same could be[come] true in other cases either now or at a later point, along with other encode/decode bugs, in a way the test simply wouldn't notice (something thats happened before).
I'd also suggest some obvious test cases that are worth adding at the same time (unless they already exist elsewhere) based on the changes being made, like an array of only empty lists (which may encode quite differently if it uses list0 type; no list count/size), and an array where an empty list isnt the first element.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] asfgit merged pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] astitcher commented on a change in pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#discussion_r448491607
##########
File path: c/src/core/encoder.c
##########
@@ -336,8 +336,10 @@ static int pni_encoder_exit(void *ctx, pn_data_t *data, pni_node_t *node)
pn_encoder_t *encoder = (pn_encoder_t *) ctx;
char *pos;
- // Special case 0 length list
- if (node->atom.type==PN_LIST && node->children-encoder->null_count==0) {
+ // Special case 0 length list, but not as first element in an array
+ pni_node_t *parent = pn_data_node(data, node->parent);
+ if (node->atom.type==PN_LIST && node->children-encoder->null_count==0 &&
+ !(pn_is_in_array(data, parent, node) && pn_is_first_in_array(data, parent, node))) {
Review comment:
I think this should test for *any* element of an array not just the first. This is because the type opcode is the array opcode for every list element and so shouldn't be changed for a 0 element list in any place in the array.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] gemmellr commented on pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#issuecomment-658125577
@kpvdr Yes, the comment applies to codec tests in general. It wouldn't surprise me if other existing test dont do it, though for me that definitely isnt reason not to do it here. More the reverse; we might not have hit some previous issues if they did, and array encodings are more complicated than most and so more likely to be broken. As they currently were, and could easily be again in other ways.
If the arrays are still using the non-list0 encoding for empty lists thats fine, I didn't look at the code enough to see what it actually did, I just think having the test of whatever its expected to be doing makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] kpvdr commented on a change in pull request #263: PROTON-2244: Fix for Array of lists with first list empty encoding
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #263:
URL: https://github.com/apache/qpid-proton/pull/263#discussion_r448993947
##########
File path: c/src/core/encoder.c
##########
@@ -336,8 +336,10 @@ static int pni_encoder_exit(void *ctx, pn_data_t *data, pni_node_t *node)
pn_encoder_t *encoder = (pn_encoder_t *) ctx;
char *pos;
- // Special case 0 length list
- if (node->atom.type==PN_LIST && node->children-encoder->null_count==0) {
+ // Special case 0 length list, but not as first element in an array
+ pni_node_t *parent = pn_data_node(data, node->parent);
+ if (node->atom.type==PN_LIST && node->children-encoder->null_count==0 &&
+ !(pn_is_in_array(data, parent, node) && pn_is_first_in_array(data, parent, node))) {
Review comment:
The type opcode for array elements is encoded only once, and is determined by the type of the first element (hence the original check). However, the size/count fields for each list element (even empty lists) must also be present as the data portion of each array element, and it is these that are omitted for all empty lists if this patch update is not made. Thanks for spotting that!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org