1Password / connect-sdk-python

Python SDK for 1Password Connect
https://developer.1password.com/docs/connect
MIT License
200 stars 31 forks source link

fix: correctly handle section objects with None type labels #49

Closed JustinTether closed 1 year ago

JustinTether commented 1 year ago

This fixes an issue we ran into where additional field items added to a 1Password vault item through the 'add more' dropdown in the GUI causes the section to be added with a None type label that looks like: 'sections': [{'id': 'add more', 'label': None}],

image

This breaks the ability to access this field using the load_dict function, as that function takes in a field path some_section.someField, splits it at the . and uses the first index in that split array as the key to access a dict that looks like {section.label: section.id}

This causes the section to never be accessible because it's impossible for path_parts[0] to be of None type

I'm unsure if this is just an edge-case that wasn't accounted for, or a possible bug in the GUI when adding a 'add more' section to a vault item, but, using the section.id in place of the label if the label happens to be None allowed us to properly access these fields again

codecov-commenter commented 1 year ago

Codecov Report

Base: 76.22% // Head: 76.24% // Increases project coverage by +0.01% :tada:

Coverage data is based on head (3199c1c) compared to base (e57fcde). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #49 +/- ## ========================================== + Coverage 76.22% 76.24% +0.01% ========================================== Files 21 21 Lines 1607 1608 +1 ========================================== + Hits 1225 1226 +1 Misses 382 382 ``` | [Impacted Files](https://codecov.io/gh/1Password/connect-sdk-python/pull/49?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/onepasswordconnectsdk/config.py](https://codecov.io/gh/1Password/connect-sdk-python/pull/49/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL29uZXBhc3N3b3JkY29ubmVjdHNkay9jb25maWcucHk=) | `93.25% <100.00%> (+0.07%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

volodymyrZotov commented 1 year ago

@JustinTether Hey. Thank you very much for contributing! I looked at your PR and it looks great!

But I would like to ask you to extend it a bit more.

Currently load_dict can work only with labels. With your fix it would work with sectionId only if there is no label set. But if there is a sectionLabel and you would try to search by sectionId - it fails.

I'd like to make load_dict work with both sectionId and sectionLabel.

"opfield": "section_id.field_label" # returns value by provided section_id and field_label

"opfield": "section_label.field_label" # returns value by provided section_label and field_label

I'd like to improve the search section flow in the next way:

  1. Try to find a section by provided string assuming it's a label (aka find by label). If found, great.
  2. If none is found, try to find a section by provided string assuming it's ID. If found, great.
  3. If none is found, throw the error.

Please let me know if you have any questions.

JustinTether commented 1 year ago

@JustinTether Hey. Thank you very much for contributing! I looked at your PR and it looks great!

But I would like to ask you to extend it a bit more.

Currently load_dict can work only with labels. With your fix it would work with sectionId only if there is no label set. But if there is a sectionLabel and you would try to search by sectionId - it fails.

I'd like to make load_dict work with both sectionId and sectionLabel.

"opfield": "section_id.field_label" # returns value by provided section_id and field_label

"opfield": "section_label.field_label" # returns value by provided section_label and field_label

I'd like to improve the search section flow in the next way:

  1. Try to find a section by provided string assuming it's a label (aka find by label). If found, great.
  2. If none is found, try to find a section by provided string assuming it's ID. If found, great.
  3. If none is found, throw the error.

Please let me know if you have any questions.

Hey @volodymyrZotov !

I've updated this commit to try and fufill your request, i've removed the patch for using section.id in place of section.label in the event that section.label is None

I've now set this up to check for a match to the section label and if that fails to also check for a match to the section id -- I've tested this locally and have been succsessful in pulling sections via their ID rather than the label

Let me know if you need anything else :grin: