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

Fix: #163. IndexError when matching values. #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ioggstream
Copy link

This PR

  • checks a value before accessing an index.

@ioggstream
Copy link
Author

cc: @dlongley @msporny is this still maintained?

@msporny
Copy link
Member

msporny commented Oct 6, 2022

cc: @dlongley @msporny is this still maintained?

Barely... it's not a priority for us because we no longer use Python. We've been searching for a maintainer that could take over maintenance of this library for several years now (to no avail).

We do plan on setting aside some money for a maintainer in the coming year... do you know of anyone that would be interested in maintaining the library? Our engineering team is too busy in other areas at the moment (VCs, DIDs, Data Integrity, etc... all building on top of JSON-LD 1.1).

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

On second thought, I'm not sure this is right when comparing it to another implementation here:

https://github.com/digitalbazaar/jsonld.js/blob/main/lib/frame.js#L813

In python, the statement not v2, when v2 is an empty array (which seems to be the case of concern here) results in True. But we are looking to return False only when v1 is actually in v2 or when v2 has a first element that is an empty object. This would erroneously return False when v2 is an empty array or isn't present (because it defaults to an empty array).

@@ -4504,7 +4504,7 @@ def _value_match(self, pattern, value):

if not v2 and not t2 and not l2:
return True
if not (v1 in v2 or _is_empty_object(v2[0])):
if not (v1 in v2 or not v2 or _is_empty_object(v2[0])):
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be:

Suggested change
if not (v1 in v2 or not v2 or _is_empty_object(v2[0])):
if not (v1 in v2 or (v2 and _is_empty_object(v2[0]))):

Ideally tests would confirm.

@ioggstream
Copy link
Author

Seems reasonable. I'll provide further comment in the code though.

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