Skip to content
This repository has been archived by the owner on Nov 19, 2023. It is now read-only.

regex to support alt application/*json types #284

Merged
merged 3 commits into from
Nov 26, 2022

Conversation

darduf
Copy link

@darduf darduf commented Nov 12, 2022

In relation to this issue - where schema tester was failing with an UndocumentedSchemaSectionError on our OAS defintitions that had application/vnd.api+json content types.

@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Merging #284 (73b7fb7) into master (1929ad3) will decrease coverage by 0.1%.
The diff coverage is 85.7%.

@@           Coverage Diff            @@
##           master    #284     +/-   ##
========================================
- Coverage    98.7%   98.5%   -0.2%     
========================================
  Files           7       7             
  Lines         474     480      +6     
  Branches       77      80      +3     
========================================
+ Hits          468     473      +5     
  Misses          4       4             
- Partials        2       3      +1     
Impacted Files Coverage Δ
openapi_tester/schema_tester.py 99.3% <85.7%> (-0.7%) ⬇️

Copy link
Member

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

Would you mind adding a test for this, to ensure no one accidentally breaks it in the future?

And just have one quick question about the logic 🙂

Comment on lines +98 to +102
if use_regex:
compiled_pattern = re.compile(key)
for key_ in schema.keys():
if compiled_pattern.match(key_):
return schema[key_]
Copy link
Member

Choose a reason for hiding this comment

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

Why are we iterating over key in schema.keys() here instead of just doing this?

if compiled_pattern.match(key):
    return schema[key_]

Copy link
Author

Choose a reason for hiding this comment

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

This is to find a str match in the schema list.
For example schema.keys() in one instance has dict_keys(['application/xml', 'application/json']) and in this case key_ will return schema["application/json"] as opposed to schema["^application\/.*json$"] (which will break).

@darduf
Copy link
Author

darduf commented Nov 12, 2022

Would you mind adding a test for this, to ensure no one accidentally breaks it in the future?

Absolutely - I was trying to understand how to add a test for this and was unsure where to add a test and what additional files are needed.
I thought of adding another OAS yaml file to ../drf-openapi-tester/tests/schemas/sample-schemas with application/vnd.api+json content. Would this suffice?
Or does it need a new test function here?
I'm not 100% sure tbh how this openapi tester runs through tests for this particular case, so I would appreciate your help on adding this test. 👍

@sondrelg
Copy link
Member

Yes you could add a new schema file, or identify a way to run the (previously) failing code directly and send in a partial schema as a dict. You should be able to test get_key_value directly and just pass a dict that contains the header.

The ideal thing, if you're comfortable with git rebasing, would be to:

  • add the test first and push that as an initial commit
  • let it run to completion and fail in the PR, to show that the old code was not working
  • then push the fix, which fixes the broken test

But don't worry about that if you're not sure how to do it 👍

@darduf darduf force-pushed the support-for-other-content-types branch from f1101c1 to d05bf91 Compare November 24, 2022 11:07
@darduf
Copy link
Author

darduf commented Nov 24, 2022

tests/test_django_framework.py:2:1: TC002 Move third-party import 'rest_framework.response.Response' into a type-checking block

To avoid this, I tried moving the import under this TYPE_CHECKING conditional as suggested.

File:./drf-openapi-tester/test_project/api/views/pets.py
08: if TYPE_CHECKING:
09:     from rest_framework.request import Request
10:     from rest_framework.response import Response

But this didn't fix, and then suggested I move it out of this block :) ?

Hopefully, we can get this PR with updated tests through soon 👍

@sondrelg
Copy link
Member

This looks good @darduf, let's try it out 👍 I'll fix the linting error

@sondrelg sondrelg merged commit 40324dc into snok:master Nov 26, 2022
@darduf
Copy link
Author

darduf commented Dec 1, 2022

It looks like this was removed in the latest release, probably from this PR as flagged here.
Can we get this back into the release please?

@sondrelg
Copy link
Member

sondrelg commented Dec 1, 2022

Yes, sorry about that 🙇 I'm flying this evening to visit family over the weekend, so I will not be free to fix this until Monday most likely. If you want to create a new PR with a fix, I will make the time to merge and release a new version, but if not I'll try to get this done then, and look at whatever other issues/PRs are remaining

@darduf
Copy link
Author

darduf commented Dec 1, 2022

So create a new PR with the exact same fix as this PR?
It would be great to get this asap if possible 🙏

@darduf
Copy link
Author

darduf commented Dec 1, 2022

Ive created a new PR - with an initial commit showing the failing test on application/json and the foloow up commit with this fix.
If you could fast track this through I would really appreciate it 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants