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

Marshmallow3 support #353

Merged
merged 31 commits into from
May 27, 2020
Merged

Conversation

cript0nauta
Copy link
Contributor

This change makes Faraday use marshmallow 3 instead of the old marshmallow 2

I based on
http://marshmallow.readthedocs.io/en/latest/upgrading.html#upgrading
The two items mentioned there were broking most of the app, fixed them
so only a few tests break
The commit d5cab0ef11ce130d58a3aafc4eb7f163e34a9543 of marshmallow broke
my custom field. (Thanks git bisect!).

See also:
marshmallow-code/marshmallow#809
marshmallow-code/marshmallow#810
Commit 7873207314cc36097db56912069a0a84d1a20de1 of marshmallow broke my
code.

See also:
marshmallow-code/marshmallow#378
marshmallow-code/marshmallow#756
This broke the vuln api in new marshmallow versions. This fix might
broke the compatibility web, but at least the tests pass.
This should be done in commit c88ab77
but I forgot to change it
Tag White v3.7.2 Fix in pink and black
rc5 changed the error messages so a few tests broke because of that
To use marshmallow 3

Command run: pypi2nixpkgs --nixpkgs https://github.com/nixos/nixpkgs/archive/91c43a9dc822da30cf3cd2908891edddcea482f2.tar.gz --local faradaysec 'pytest-factoryboy>=2.0.1'
- print without parens (this was python 2 code)
- Usage of strict=True (not valid anymore in marshmallow 3)
- Usage of both missing and required=True in a schema (this didn't make
  sense)

Now all tests are running, but most are failing
Allow passing strict=True to AutoSchema, even if it's a useless
argument. And fix bugs with schemas that weren't based on AutoSchema
It was duplicated with the "query" field and this caused marshmallow to
fail when filtering. Disable that field until we find a solution for
this.
Add a compatibility layer in AutoSchema to be able to use the .data of
the result of dump and load. Remove .data access manually in non
AutoSchema based schemas.
It was not compatible with marshmallow 3 because it was returning None:

"In marshmallow 2.x, None returned by a pre or post-processor is
interpreted as “the data was mutated”. In marshmallow 3.x, the return
value is considered as processed data even if it is None."
I didn't note this because the ReportProcessor runs in a thread inside
the tests, so it didn't directly raise an exception (but it was logged)
I forgot to use bytes instead of str when resolving a merge conflict
* Add `**kwargs` to decorated schema methods, as detailed in
  https://marshmallow.readthedocs.io/en/stable/changelog.html#rc7-2019-06-15
* Add `**kwargs` to `_deserialize` method of custom fields, as detailed in
  https://marshmallow.readthedocs.io/en/stable/upgrading.html#custom-fields
* Upgrade marshmallow-sqlalchemy because 0.15.0 is not compatible with
  this marshmallow version. (Note: I'm ignoring the problem described in
  #5687)
* Don't use marshmallow.compat. It was designed for python 2/3
  compatibility and was removed in 3.0.0rc7
* Fix a strange bug when using PolymorphicVulnerabilityField
* Use keyword-only arguments instead of positional arguments when
  needed
I needed to add a horrible hack to maintain backwards compatibility in
the API DateTime fields. I prefer to have this horrible thing but ensure
our API works in the same way as before.
The vulns API must accept query_string and query, params and parameters,
etc.
Because of the DictWithData hack, a piece of code got the vuln data
field instead of the full JSON with all vuln fields.

Note: this bug only happened in our commercial versions, not in
community.
@cript0nauta cript0nauta merged commit 77dc49f into infobyte:master May 27, 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.

1 participant