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

Upgrade marshmallow and marshmallow-sqlalchemy #1224

Closed
9 tasks done
kalbfled opened this issue Apr 19, 2023 · 6 comments
Closed
9 tasks done

Upgrade marshmallow and marshmallow-sqlalchemy #1224

kalbfled opened this issue Apr 19, 2023 · 6 comments
Assignees
Labels

Comments

@kalbfled
Copy link
Member

kalbfled commented Apr 19, 2023

User Story - Business Need

While working on #1222, upgrading marshmallow and marshmallow-sqlalchemy to the latest major versions resulted in successful build followed by runtime errors in multiple container. The tracebacks concluded with ValueError: Couldn't import '<varies by container>': Invalid fields for <UserSchema(many=False)>: {'user_to_organisation', 'verify_codes_identity_provider_user_id', 'user_to_service'}.

  • discuss w/ QA

User Story(ies)

As a VA Notify developer
I want to upgrade marshmallow and marshmallow-sqlalchemy
So that we minimize vulnerabilities and future upgrade pains.

Additional Info and Resources

Appears to be up to 9 classes/objects that would need tweaking:
Private Zenhub Image

There are numerous instances of this SAWarning which is coming from marshmallow-sqlalchemy; check to either make sure these aren't seen or refactor to remove these warnings.

/usr/local/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py:170: SAWarning: relationship 'ScheduledNotification.notification' will copy column notifications.id to column scheduled_notifications.notification_id, which conflicts with relationship(s): 'Notification.scheduled_notification' (copies notifications.id to scheduled_notifications.notification_id). If this is not the intention, consider if these relationships should be linked with back_populates, or if viewonly=True should be applied to one or more if they are read-only. For the less common case that foreign key constraints are partially overlapping, the orm.foreign() annotation can be used to isolate the columns that should be written towards.   To silence this warning, add the parameter 'overlaps="scheduled_notification"' to the 'ScheduledNotification.notification' relationship. (Background on this error at: https://sqlalche.me/e/14/qzyx)

Engineering Checklist

  • Upgrade the effected packages in requirements-app.txt
  • Build a new Docker image
  • Run the app locally. Observe failures. Fix them.
  • Update requirements.txt as described in README.md
  • Update the Slide deck with the key win and a demo slide

Acceptance Criteria

  • The image builds
  • All unit tests pass with the latest versions of the effected packages

QA Considerations

  • QA regression passes

Potential Dependencies

Leave blank if n/a

Out of Scope

Do not upgrade Celery or SQLAlchemy. Separate tickets exist for those upgrades.

@mjones-oddball
Copy link

Hey team! Please add your planning poker estimate with Zenhub @cris-oddball @justaskdavidb2 @k-macmillan @kalbfled @ldraney @nikolai-efimov

@npmartin-oddball
Copy link

Please add your planning poker estimate with Zenhub @EvanParish

@mjones-oddball
Copy link

While working on #1225 @kalbfled realized we may need to also upgrade marshmallow to get past certain errors

@kalbfled
Copy link
Member Author

kalbfled commented Mar 19, 2024

Please see #1225 for comments relevant to this ticket. A draft PR exists to address both issues.

@mchlwellman
Copy link

There are currently 63 test failing -

FAILED tests/app/notifications/rest/test_send_notification.py::test_should_not_send_notification_if_restricted_and_not_a_service_user[[email protected]] - TypeError: 'NoneType' object is not subscriptable
FAILED tests/app/notifications/rest/test_send_notification.py::test_should_not_allow_template_from_another_service[email] - assert 400 == 404
FAILED tests/app/notifications/rest/test_send_notification.py::test_should_reject_email_notification_with_bad_email - TypeError: 'NoneType' object is not subscriptable
FAILED tests/app/notifications/rest/test_send_notification.py::test_should_not_send_notification_to_non_whitelist_recipient_in_trial_mode[[email protected]] - TypeError: 'NoneType' object is not subscriptable
FAILED tests/app/notifications/rest/test_send_notification.py::test_should_not_send_notification_to_non_whitelist_recipient_in_trial_mode[[email protected]] - TypeError: 'NoneType' object is not subscriptable
FAILED tests/app/notifications/rest/test_send_notification.py::test_should_error_if_notification_type_does_not_match_template_type[[email protected]] - TypeError: argument of type 'NoneType' is not iterable
FAILED tests/app/dao/test_templates_dao.py::test_get_template_versions - marshmallow.exceptions.ValidationError: {0: {'_schema': ['Invalid input type.']}, 1: {'_schema': ['Invalid input type.']}}
FAILED tests/app/notifications/rest/test_send_notification.py::test_create_notification_should_reject_if_missing_required_fields[sms] - TypeError: 'NoneType' object is not subscriptable
FAILED tests/app/notifications/rest/test_send_notification.py::test_create_notification_should_reject_if_missing_required_fields[email] - TypeError: 'NoneType' object is not subscriptable
FAILED tests/app/notifications/rest/test_send_notification.py::test_should_reject_bad_phone_numbers - AttributeError: 'NoneType' object has no attribute 'keys'
FAILED tests/app/notifications/rest/test_send_notification.py::test_send_notification_invalid_template_id[[email protected]] - assert 400 == 404
FAILED tests/app/service/test_rest.py::test_update_service_sets_volumes[None-200-None-volume_letter] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_sets_research_consent[True-200-True] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_sets_research_consent[False-200-False] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_create_service[True-False] - assert 400 == 201
FAILED tests/app/service/test_rest.py::test_update_service_flags_with_service_without_default_service_permissions - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_create_service[False-True] - assert 400 == 201
FAILED tests/app/service/test_rest.py::test_should_error_if_created_by_missing - KeyError: 'created_by'
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_missing_if_user_id_is_not_in_database - assert 400 == 404
FAILED tests/app/service/test_rest.py::test_update_service_flags_will_remove_service_permissions - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_permissions_will_override_permission_flags - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_should_not_create_service_if_missing_data - KeyError: 'name'
FAILED tests/app/service/test_rest.py::test_update_service_permissions_will_add_service_permissions - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_duplicate_name - KeyError: 'name'
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_non_existent_provider[email] - KeyError: 'email_provider_id'
FAILED tests/app/service/test_rest.py::test_add_service_permission_will_add_permission[email] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_non_existent_provider[sms] - KeyError: 'sms_provider_id'
FAILED tests/app/service/test_rest.py::test_add_service_permission_will_add_permission[sms] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_inactive_provider[email] - KeyError: 'email_provider_id'
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_inactive_provider[sms] - KeyError: 'sms_provider_id'
FAILED tests/app/service/test_rest.py::test_add_service_permission_will_add_permission[international_sms] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_incorrect_provider_notification_type[email] - KeyError: 'email_provider_id'
FAILED tests/app/service/test_rest.py::test_add_service_permission_will_add_permission[letter] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_default_permissions_are_added_for_user_service - assert 400 == 201
FAILED tests/app/service/test_callback_rest.py::TestUpdateServiceCallback::test_update_service_callback_modifies_updated_at - AssertionError: assert '2021-05-13T12:00:00' == '2021-05-13T12:00:00+00:00'
FAILED tests/app/service/test_rest.py::test_should_not_create_service_with_incorrect_provider_notification_type[sms] - KeyError: 'sms_provider_id'
FAILED tests/app/service/test_rest.py::test_add_service_permission_will_add_permission[inbound_sms] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_permissions_with_an_invalid_permission_will_raise_error - KeyError: 'permissions'
FAILED tests/app/service/test_rest.py::test_should_not_update_service_with_inactive_provider[email] - KeyError: 'email_provider_id'
FAILED tests/app/service/test_rest.py::test_update_service_calls_send_notification_as_service_becomes_live - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_permissions_with_duplicate_permissions_will_raise_error - KeyError: 'permissions'
FAILED tests/app/service/test_rest.py::test_should_not_update_service_with_inactive_provider[sms] - KeyError: 'sms_provider_id'
FAILED tests/app/service/test_rest.py::test_update_service_does_not_call_send_notification_for_live_service - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_research_mode_throws_validation_error - KeyError: 'research_mode'
FAILED tests/app/service/test_rest.py::test_update_service_does_not_call_send_notification_when_restricted_not_changed - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_should_not_update_service_with_duplicate_name - KeyError: 'name'
FAILED tests/app/service/test_rest.py::test_should_not_update_service_with_nonexistent_provider[email] - KeyError: 'email_provider_id'
FAILED tests/app/service/test_rest.py::test_search_for_notification_by_to_field_filters_by_status - KeyError: 'notifications'
FAILED tests/app/service/test_rest.py::test_should_not_update_service_with_nonexistent_provider[sms] - KeyError: 'sms_provider_id'
FAILED tests/app/service/test_rest.py::test_cant_update_service_org_type_to_random_value - assert 400 == 500
FAILED tests/app/service/test_rest.py::test_search_for_notification_by_to_field_filters_by_statuses - KeyError: 'notifications'
FAILED tests/app/service/test_rest.py::test_update_service_flags - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_sets_volumes[1234-200-1234-volume_email] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_set_sms_prefixing_for_service[True-True-True] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_sets_volumes[1234-200-1234-volume_sms] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_set_sms_prefixing_for_service[False-False-False] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_sets_volumes[1234-200-1234-volume_letter] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_set_sms_prefixing_for_service_cant_be_none - AssertionError: assert {'_schema': [...input type.']} == {'prefix_sms'...ot be null.']}
FAILED tests/app/service/test_rest.py::test_update_service_sets_volumes[None-200-None-volume_email] - assert 400 == 200
FAILED tests/app/service/test_rest.py::test_update_service_sets_volumes[None-200-None-volume_sms] - assert 400 == 200
FAILED tests/app/test_schemas.py::test_services_schema_includes_providers - KeyError: 'email_provider_id'
FAILED tests/app/template/test_rest.py::test_update_does_not_create_new_version_when_there_is_no_change - assert 2 == 1
============================= 63 failed, 2812 passed, 58 skipped, 17 xfailed in 63.87s (0:01:03) ==============================
Unit tests failed

@cris-oddball
Copy link

PR approved and merged. All non-pinned dependencies were updated, but no Twistlock issues resolved. 1 dependabot issue was resolved.

Sending to Perf.

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

No branches or pull requests

8 participants