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/06/02 17:21:57 UTC
[GitHub] [qpid-proton] kpvdr opened a new pull request #256: PROTON-2237: Correct checking of Proton message property keys
kpvdr opened a new pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256
Proposed fix for PROTON-2237.
----------------------------------------------------------------
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] codecov-commenter edited a comment on pull request #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#issuecomment-637742528
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=h1) Report
> Merging [#256](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=desc) into [master](https://codecov.io/gh/apache/qpid-proton/commit/65a5d9e837cf8de3024e7039dc7f5f2175d13461&el=desc) will **increase** coverage by `0.09%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 83.05% 83.14% +0.09%
==========================================
Files 338 339 +1
Lines 41431 41521 +90
==========================================
+ Hits 34410 34523 +113
+ Misses 7021 6998 -23
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [python/proton/\_message.py](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-cHl0aG9uL3Byb3Rvbi9fbWVzc2FnZS5weQ==) | `86.69% <100.00%> (ø)` | |
| [ruby/tests/test\_interop.rb](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-cnVieS90ZXN0cy90ZXN0X2ludGVyb3AucmI=) | `95.55% <0.00%> (ø)` | |
| [c/src/proactor/epoll.c](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-Yy9zcmMvcHJvYWN0b3IvZXBvbGwuYw==) | `82.25% <0.00%> (+0.99%)` | :arrow_up: |
| [ruby/lib/codec/data.rb](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-cnVieS9saWIvY29kZWMvZGF0YS5yYg==) | `96.10% <0.00%> (+3.89%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=footer). Last update [65a5d9e...ccbfcf6](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469348071
##########
File path: python/proton/_message.py
##########
@@ -90,10 +90,12 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- # Check for string type. (py2: unicode, py3: str via type hack above)
- # String subclasses symbol and char are excluded
- # (But so are other string subclasses that would be encoded as type string!)
- if type(k) == unicode:
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses
+ if isinstance(k, unicode):
+ # Convert string subclasses (including proton char and symbol types) to string
Review comment:
Now that we are specifically excluding symbol and char types, your code applies.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r436066742
##########
File path: python/proton/_message.py
##########
@@ -90,10 +90,12 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- # Check for string type. (py2: unicode, py3: str via type hack above)
- # String subclasses symbol and char are excluded
- # (But so are other string subclasses that would be encoded as type string!)
- if type(k) == unicode:
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses
+ if isinstance(k, unicode):
+ # Convert string subclasses (including proton char and symbol types) to string
Review comment:
I really don't understand why this is better than
```if isinstance(k, unicode) and not type(k) is symbol:```
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,16 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string type. (py2: unicode, py3: str via type hack above)
+ # String subclasses symbol and char are excluded
+ # (But so are other string subclasses that would be encoded as type string!)
+ if type(k) == unicode:
Review comment:
I think do the opposite - check for the specific single type symbol and error out for it - every other string type is allowed. @ssorj what do you think?
##########
File path: python/proton/_message.py
##########
@@ -90,10 +90,12 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- # Check for string type. (py2: unicode, py3: str via type hack above)
- # String subclasses symbol and char are excluded
- # (But so are other string subclasses that would be encoded as type string!)
- if type(k) == unicode:
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses
+ if isinstance(k, unicode):
+ # Convert string subclasses (including proton char and symbol types) to string
+ if not type(k) == unicode:
Review comment:
Note that ```==``` is not the correct test here ```is``` would be the correct test.
----------------------------------------------------------------
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 closed pull request #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr closed pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469349354
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,16 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string type. (py2: unicode, py3: str via type hack above)
+ # String subclasses symbol and char are excluded
+ # (But so are other string subclasses that would be encoded as type string!)
+ if type(k) == unicode:
Review comment:
After discussion, we think that AMQP subclasses of string should be excluded from the conversion. Thus, symbol and char will cause an exception, but other user-defined subclasses will be converted.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469505961
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
Review comment:
Agreed. Some text would be helpful.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469511394
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
Review comment:
OK, you are ahead of me. Given the if / elif / else structure, removing 'continue' would be logically the same, I think.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r448484945
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,16 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string type. (py2: unicode, py3: str via type hack above)
+ # String subclasses symbol and char are excluded
+ # (But so are other string subclasses that would be encoded as type string!)
+ if type(k) == unicode:
Review comment:
I did it this way on the understanding that we convert ALL unicode subclasses, including symbol and char, and that this would add to an easier user experience. Certainly it would be more "technically correct" to exclude symbol and char though.
----------------------------------------------------------------
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] codecov-commenter commented on pull request #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#issuecomment-637742528
# [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=h1) Report
> Merging [#256](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=desc) into [master](https://codecov.io/gh/apache/qpid-proton/commit/65a5d9e837cf8de3024e7039dc7f5f2175d13461&el=desc) will **increase** coverage by `0.09%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 83.05% 83.14% +0.09%
==========================================
Files 338 339 +1
Lines 41431 41521 +90
==========================================
+ Hits 34410 34523 +113
+ Misses 7021 6998 -23
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [python/proton/\_message.py](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-cHl0aG9uL3Byb3Rvbi9fbWVzc2FnZS5weQ==) | `86.69% <100.00%> (ø)` | |
| [ruby/tests/test\_interop.rb](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-cnVieS90ZXN0cy90ZXN0X2ludGVyb3AucmI=) | `95.55% <0.00%> (ø)` | |
| [c/src/proactor/epoll.c](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-Yy9zcmMvcHJvYWN0b3IvZXBvbGwuYw==) | `82.25% <0.00%> (+0.99%)` | :arrow_up: |
| [ruby/lib/codec/data.rb](https://codecov.io/gh/apache/qpid-proton/pull/256/diff?src=pr&el=tree#diff-cnVieS9saWIvY29kZWMvZGF0YS5yYg==) | `96.10% <0.00%> (+3.89%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=footer). Last update [65a5d9e...ccbfcf6](https://codecov.io/gh/apache/qpid-proton/pull/256?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469510090
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
Review comment:
I think it is a way of jumping out of the code within the for loop and continuing with the loop itself:
```
for k in self.property.keys():
if <k is unicode or its subclasses, excluding symbol and char>:
if <k is a subclass of unicode>:
<convert key to unicode>
continue
other stuff...
```
is a way of avoiding other stuff if <k is unicode or its subclasses, excluding symbol and char> is true. But I am sure there is a way of rearranging this to avoid it. Certainly it is not possible to replace continue with pass, that is not logically the same, as other stuff would also then be executed.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469561876
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
Review comment:
No, encode won't work directly on subclasses of unicode. Data.put_mappings throws a KeyError, as the subclass name is not known.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#issuecomment-673121713
This should be close now. Passes both Py2 and Py3 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 a change in pull request #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469504092
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
Review comment:
This is to make the change only if the key is a subclass of unicode, but not unicode itself. As we first check with isinstance(unicode), all unicode and their subclasses are included, and continue will be called. But for sublcasses only, a conversion is made, hence the second test using type() is unicode.
----------------------------------------------------------------
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] ssorj commented on pull request #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
ssorj commented on pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#issuecomment-738999720
@kpvdr would you close this one if it's ready? Note that there is a comment notation that will close PRs for you.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469505140
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
- # If key is binary then change to string
- elif isinstance(k, str):
- # py2 str
- self.properties[k.encode('utf-8')] = self.properties.pop(k)
+ # If key is binary then change to string. Exclude bytes subclass decimal128.
+ # Mostly for py2 users who encode strings without using the u'' prefix
+ # but py3 bytes() type will be converted also
Review comment:
I have thought of this, and it is a valid point. It was only for Py2 users which did not use the u'' prefix that this conversion was included. For Py3 users, this could and probably should be an error.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#issuecomment-672945072
@jiridanek: Yes, I just added them.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256
----------------------------------------------------------------
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] jiridanek commented on pull request #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#issuecomment-665112300
@kpvdr Do you have a test for this change? I think it should be part of the PR.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r448482190
##########
File path: python/proton/_message.py
##########
@@ -90,10 +90,12 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- # Check for string type. (py2: unicode, py3: str via type hack above)
- # String subclasses symbol and char are excluded
- # (But so are other string subclasses that would be encoded as type string!)
- if type(k) == unicode:
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses
+ if isinstance(k, unicode):
+ # Convert string subclasses (including proton char and symbol types) to string
+ if not type(k) == unicode:
Review comment:
Agreed.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469565080
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
- # If key is binary then change to string
- elif isinstance(k, str):
- # py2 str
- self.properties[k.encode('utf-8')] = self.properties.pop(k)
+ # If key is binary then change to string. Exclude bytes subclass decimal128.
+ # Mostly for py2 users who encode strings without using the u'' prefix
+ # but py3 bytes() type will be converted also
+ elif isinstance(k, bytes) and not (type(k) is decimal128):
Review comment:
Good point. Agreed.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r448481904
##########
File path: python/proton/_message.py
##########
@@ -90,10 +90,12 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- # Check for string type. (py2: unicode, py3: str via type hack above)
- # String subclasses symbol and char are excluded
- # (But so are other string subclasses that would be encoded as type string!)
- if type(k) == unicode:
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses
+ if isinstance(k, unicode):
+ # Convert string subclasses (including proton char and symbol types) to string
Review comment:
It is not logically identical.
We want the continue to be reached for all unicode. However, only for subclasses of unicode, we make the conversion.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469510090
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
Review comment:
I think it is a way of jumping out of the code within the for loop and continuing with the loop itself:
```
for k in self.property.keys():
if <k is unicode or its subclasses, excluding symbol and char>:
if <k is a subclass of unicode>:
<convert key to unicode>
continue
other stuff...
```
is a way of avoiding other stuff if xxx is true. But I am sure there is a way of rearranging this to avoid it. Certainly it is not possible to replace continue with pass, that is not logically the same, as other stuff would also then be executed.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
astitcher commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469470459
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
Review comment:
It would be really helpful here to explain what the AMQP limitations on property keys are. I can't figure out from reading this code what the actual point of the code is.
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
Review comment:
But if the above code is actually necessary for some reason, then this line is no longer necessary. As in context it is just another way of saying 'pass'
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
Review comment:
Why is this even necessary? It's already a (perhaps subclass of) unicode. won't encode just work directly?
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
- # If key is binary then change to string
- elif isinstance(k, str):
- # py2 str
- self.properties[k.encode('utf-8')] = self.properties.pop(k)
+ # If key is binary then change to string. Exclude bytes subclass decimal128.
+ # Mostly for py2 users who encode strings without using the u'' prefix
+ # but py3 bytes() type will be converted also
Review comment:
I don't think converting py3 bytes is acceptable as binary is not a good key (or is it?) it looks like in the previous code it would have triggered an error - only py2 binary would have been converted.
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
- # If key is binary then change to string
- elif isinstance(k, str):
- # py2 str
- self.properties[k.encode('utf-8')] = self.properties.pop(k)
+ # If key is binary then change to string. Exclude bytes subclass decimal128.
+ # Mostly for py2 users who encode strings without using the u'' prefix
+ # but py3 bytes() type will be converted also
+ elif isinstance(k, bytes) and not (type(k) is decimal128):
Review comment:
I think you can leave this test as 'str' as it was before - it only covers the py2 str (binary) case as py3 str is already covered above in unicode, I guess you still need the decimal128 case still though.
----------------------------------------------------------------
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 #256: PROTON-2237: Correct checking of Proton message property keys
Posted by GitBox <gi...@apache.org>.
kpvdr commented on a change in pull request #256:
URL: https://github.com/apache/qpid-proton/pull/256#discussion_r469565080
##########
File path: python/proton/_message.py
##########
@@ -90,13 +90,18 @@ def _check(self, err):
def _check_property_keys(self):
for k in self.properties.keys():
- if isinstance(k, unicode):
- # py2 unicode, py3 str (via hack definition)
+ # Check for string types. (py2: unicode, py3: str via type hack above)
+ # or string subclasses. Exclude string subclasses symbol and char.
+ if isinstance(k, unicode) and not (type(k) is symbol or type(k) is char):
+ # Convert string subclasses to string
+ if not type(k) is unicode:
+ self.properties[unicode(k)] = self.properties.pop(k)
continue
- # If key is binary then change to string
- elif isinstance(k, str):
- # py2 str
- self.properties[k.encode('utf-8')] = self.properties.pop(k)
+ # If key is binary then change to string. Exclude bytes subclass decimal128.
+ # Mostly for py2 users who encode strings without using the u'' prefix
+ # but py3 bytes() type will be converted also
+ elif isinstance(k, bytes) and not (type(k) is decimal128):
Review comment:
It does not work because the assumption that py3 strings are already covered is not valid. If the key is a symbol or char, they are excluded from the above case, and they will pass isinstance(k, str) and be converted here, resulting in an error.
----------------------------------------------------------------
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