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

Correct out-of-spec Avro schemas with typing.Union #87

Merged
merged 9 commits into from
Dec 4, 2020

Conversation

xgamer4
Copy link
Contributor

@xgamer4 xgamer4 commented Nov 19, 2020

This pull request corrects #85 by checking the type of the provided default and placing it at the front of the type field in Avro schema. It adds two tests - one to reproduce the problem detailed in Issue 85 to develop against, and one to add a test to explicitly document the behavior of a typing.Optional[T] field without a default. The behavior of a typing.Optional[T] field remains unchanged. The existing test for union fields with defaults was updated.

Documentation still needs to be updated, and notes on the change (it's likely a breaking change, and notes on the severity) need to be added.

I wouldn't be surprised if there were minor merge conflicts with #86 as well - the two branches were developed independently of each other - so I'll need to reconcile those if/when one of these two merge requests gets approved.

@xgamer4
Copy link
Contributor Author

xgamer4 commented Nov 20, 2020

Testing against the released v0.19.0 for breaking changes, I built a class like this:

@dataclass
class RandomSentence(faust.Record, AvroModel, serializer='avro_randomsentence'):
    """Randomly Generated Sentence. Purely for testing during initial dev"""
    sentence: str
    generated: datetime
    test: typing.Optional[str] = None
    test2: typing.Union[str, int] = 0

Ran code against it to have the schema version saved to the schema registry (under full compatibility). I then altered the model to this, to emulate the changes made under this pull request:

@dataclass
class RandomSentence(faust.Record, AvroModel, serializer='avro_randomsentence'):
    """Randomly Generated Sentence. Purely for testing during initial dev"""
    sentence: str
    generated: datetime
    test: typing.Union[None, str] = None
    test2: typing.Union[int, str] = 0

And the change went through and was saved to the schema registry as a new version, as appropriate.

@xgamer4 xgamer4 marked this pull request as ready for review November 20, 2020 17:16
keep my branches up to date
# Conflicts:
#	tests/schemas/conftest.py
@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #87 (e4f55cb) into master (edfa15d) will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   99.55%   99.85%   +0.29%     
==========================================
  Files           7        7              
  Lines         671      678       +7     
  Branches      106      108       +2     
==========================================
+ Hits          668      677       +9     
+ Misses          1        0       -1     
+ Partials        2        1       -1     
Impacted Files Coverage Δ
dataclasses_avroschema/fields.py 100.00% <100.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edfa15d...e4f55cb. Read the comment docs.

@marcosschroh
Copy link
Owner

@xgamer4 amazing! Nice work 👍 . Do you want to update the documentation as well?

@xgamer4
Copy link
Contributor Author

xgamer4 commented Dec 1, 2020

@marcosschroh Thanks! Documentation has been updated - I added in a bit of explanation to the complex types section on Unions and updated the example.

EDIT: After a bit of thought I added in a note on non-breaking changes that generate a new schema version below the note on breaking changes. In practice I'm not sure if those matter, but it seemed worth noting that a new version will wind up updating schema versions without any changes on the dev's side.

@marcosschroh marcosschroh merged commit 972799f into marcosschroh:master Dec 4, 2020
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.

None yet

3 participants