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

feat: Support union schemas #1525

Merged
merged 27 commits into from
Jun 27, 2023
Merged

feat: Support union schemas #1525

merged 27 commits into from
Jun 27, 2023

Conversation

@edgarrmondragon edgarrmondragon changed the title feat: Support union schemas WIP: Support union schemas Mar 24, 2023
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #1525 (650c18c) into main (c6a8933) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head 650c18c differs from pull request most recent head 7384302. Consider uploading reports for the commit 7384302 to get more accurate results

@@            Coverage Diff             @@
##             main    #1525      +/-   ##
==========================================
- Coverage   86.12%   85.98%   -0.14%     
==========================================
  Files          59       59              
  Lines        4944     4959      +15     
  Branches      809      811       +2     
==========================================
+ Hits         4258     4264       +6     
- Misses        485      492       +7     
- Partials      201      203       +2     
Impacted Files Coverage Δ
singer_sdk/typing.py 97.00% <100.00%> (+0.23%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@edgarrmondragon edgarrmondragon changed the title WIP: Support union schemas feat: Support union schemas Mar 24, 2023
@edgarrmondragon edgarrmondragon marked this pull request as ready for review March 24, 2023 21:07
@edgarrmondragon
Copy link
Collaborator Author

edgarrmondragon commented Mar 24, 2023

@ReubenFrankel I'd love to get your thoughts on this? Is it helpful, intuitive?

@ReubenFrankel
Copy link
Contributor

@edgarrmondragon I think this looks great - really like the syntax! I've spent a while thinking about what this might not cover with more complex cases, but I think its better to tackle those as and when they come up, since the only scenarios I can think of at the moment are pretty unrealistic. My only question is what does a validation error for this look like? Is it helpful to the user?

This would be great to get into tap-github once it's merged (see Accepted Config Options) - OneOf is perfect for the mode options (i.e. repositories, organizations, searches, etc).

@edgarrmondragon
Copy link
Collaborator Author

My only question is what does a validation error for this look like? Is it helpful to the user?

@ReubenFrankel The default jsonschema exception message is not the most readable:

jsonschema.exceptions.ValidationError: {'flow': 'oauth', 'client_id': '123', 'username': '456', 'password': '789'} is not valid under any of the given schemas

Failed validating 'oneOf' in schema:
    {'oneOf': [{'additionalProperties': False,
                'properties': {'client_id': {'secret': True,
                                             'type': ['string'],
                                             'writeOnly': True},
                               'client_secret': {'secret': True,
                                                 'type': ['string'],
                                                 'writeOnly': True},
                               'flow': {'const': 'oauth',
                                        'description': 'Discriminator for '
                                                       'object of type '
                                                       "'oauth'."}},
                'required': ['flow', 'client_id', 'client_secret'],
                'type': 'object'},
               {'additionalProperties': False,
                'properties': {'flow': {'const': 'password',
                                        'description': 'Discriminator for '
                                                       'object of type '
                                                       "'password'."},
                               'password': {'secret': True,
                                            'type': ['string'],
                                            'writeOnly': True},
                               'username': {'type': ['string']}},
                'required': ['flow', 'username', 'password'],
                'type': 'object'}]}

On instance:
    {'client_id': '123',
     'flow': 'oauth',
     'password': '789',
     'username': '456'}

But we could of course grab the context and print something that better explains the error and expectation. I have a PR where it might make sense to add that: #768.

@edgarrmondragon edgarrmondragon removed the request for review from aaronsteers April 18, 2023 21:07
@edgarrmondragon
Copy link
Collaborator Author

@kgpayne can you give this PR a look when get a chance?

@edgarrmondragon edgarrmondragon removed the request for review from cjohnhanson May 31, 2023 17:01
@kgpayne kgpayne enabled auto-merge (squash) June 27, 2023 14:13
@kgpayne kgpayne disabled auto-merge June 27, 2023 15:03
@kgpayne kgpayne enabled auto-merge (squash) June 27, 2023 15:04
@kgpayne kgpayne merged commit d81c3f7 into main Jun 27, 2023
@kgpayne kgpayne deleted the feat/one-of-discriminator branch June 27, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants