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.

Sign up for GitHub

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

Jump to bottom

KK-1108 | Upgrade to Django 4.2 & upgrade all packages to latest #390

Merged
merged 10 commits into from
Aug 23, 2024
Merged

KK-1108 | Upgrade to Django 4.2 & upgrade all packages to latest #390

merged 10 commits into from
Aug 23, 2024

Conversation

karisal-anders
Copy link
Contributor

@karisal-anders karisal-anders commented Jun 19, 2024 •
edited
Loading

Description

REQUIRES PIPELINES TO BE CHANGED FROM POSTGRES 11 TO 12 in these files:

by changing image: postgres:11 to image: postgres:12.

feat: upgrade Django to 4.2 & upgrade all packages to latest

done:

  • upgrade postgres from 11 to 12
    • required, didn't work without this change
  • change import paths
    • graphql.execution.base.ResolveInfo ->
      graphql.type.GraphQLResolveInfo
  • add arbitrary primary keys to objects used in dummy notification
    contexts because factory's build doesn't create a primary key yet,
    and without it generating the URLs using the object's ID fails
  • remove QUERY_TOO_DEEP_ERROR and DepthAnalysisBackend, use graphene's
    depth_limit_validator instead which should give a GraphQLError like
    `<operation_name> exceeds maximum operation depth of <max_depth>.
  • fix executed_graphql_request() has no invalid member
  • fix graphene enums to work similarly to graphene v2 -> using values
    • graphene v3 changed enums so they work differently than in
      graphene v2, forcing backward compatibility by converting enums
      to their values
  • fix using django-parler translations, map enums to their values
  • fix enums in TranslatableQuerySet objects, map them to their values

refs KK-1108

Related

KK-1108

Graphene 3 changed from using enum values to enums, for example:

class TestEnum(graphene.Enum):
  FIRST = 'first'
  SECOND = 'second'

would've been serialized previously using the enum values, i.e.
'first' and 'second'. But with Graphene 3 they are serialized as
'TestEnum.FIRST' and 'TestEnum.SECOND'. This broke functionality as
parts of the codebase were expecting the enum values as per Graphene 2.

Forced backwards compatibility by forcefully mapping enums to their
values.

Related https://github.com/graphql-python/graphene issues & PRs:

See https://github.com/graphql-python/graphene/wiki/v3-release-notes
for Graphene v3's breaking changes,

Manual testing

Running pytest . -vv in docker container locally everything passes:

collected 815 items
...
248 snapshots passed.
=========== 815 passed, 11 warnings in 88.08s (0:01:28) ===========

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@karisal-anders karisal-anders force-pushed the KK-1108-upgrade-django-4-2 branch from 642aa6d to 66044b0 Compare June 20, 2024 14:09
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@karisal-anders karisal-anders force-pushed the KK-1108-upgrade-django-4-2 branch from 66044b0 to 09a3d02 Compare July 8, 2024 12:17
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@karisal-anders karisal-anders force-pushed the KK-1108-upgrade-django-4-2 branch from 4e3d3e7 to 572e845 Compare July 11, 2024 14:13
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@karisal-anders karisal-anders force-pushed the KK-1108-upgrade-django-4-2 branch from 5a43a5e to fa9e434 Compare July 15, 2024 14:05
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@karisal-anders karisal-anders marked this pull request as ready for review July 16, 2024 13:53
@karisal-anders
feat: upgrade Django to 4.2 & upgrade all packages to latest
f9a8200 
done:
 - upgrade postgres from 11 to 12
   - required, didn't work without this change
 - change import paths
   - graphql.execution.base.ResolveInfo ->
     graphql.type.GraphQLResolveInfo
 - add arbitrary primary keys to objects used in dummy notification
   contexts because factory's build doesn't create a primary key yet,
   and without it generating the URLs using the object's ID fails
 - remove QUERY_TOO_DEEP_ERROR and DepthAnalysisBackend, use graphene's
   depth_limit_validator instead which should give a GraphQLError like
   `<operation_name> exceeds maximum operation depth of <max_depth>.
 - fix executed_graphql_request() has no `invalid` member
 - fix graphene enums to work similarly to graphene v2 -> using values
   - graphene v3 changed enums so they work differently than in
     graphene v2, forcing backward compatibility by converting enums
     to their values
 - fix using django-parler translations, map enums to their values
 - fix enums in TranslatableQuerySet objects, map them to their values

refs KK-1108
@karisal-anders karisal-anders force-pushed the KK-1108-upgrade-django-4-2 branch from 6a3f567 to f9a8200 Compare July 17, 2024 10:25
@karisal-anders karisal-anders changed the title (DRAFT) KK-1108 | Upgrade to Django 4.2 KK-1108 | Upgrade to Django 4.2 Jul 17, 2024
@karisal-anders karisal-anders mentioned this pull request Jul 17, 2024
Merged
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@nikomakela nikomakela mentioned this pull request Aug 20, 2024
Merged
@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

@nikomakela
chore: init postgres data volume to version specific location
045300f 
KK-1108
1. The version property is now obsoleted in the docker compose config.
2. Since the Postgre is upgraded with a new major release, the database
will need a migration for the existing data. To skip this, we can create
a new database in different volume location. This is quite common
aproach.
@nikomakela
refactor: datetime imports of the children schema
395157e 
KK-1108.
@nikomakela
refactor: remove needless datetime import
135b0d0 
KK-1108
@nikomakela
feat: add more fields and filters to messaging admin view
105a15b 
KK-1108
@nikomakela
fix: message api filters for protocol and occurrences
97bd17c 
KK-1108.
Re-add the protocol and occurrences filters that were earlier removed,
because they were hard to fix (during the upgrade process).
@nikomakela
Copy link
Contributor

nikomakela commented Aug 20, 2024 •
edited
Loading

I think the piipeline changes are only thing that is still missing from the API update. Sure we need to offer supportive schema for both the UI clients too!

  • Do the pipeline configuration changes
  • Offer schema update for the Public UI
  • Offer schema update for the Admin UI

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is failed for https://kukkuu-pr390.api.dev.hel.ninja 😿💢💥💥

This was referenced Aug 22, 2024
Merged
Merged
@nikomakela
fix: event group creation bug in admin
501fb82 
KK-1108.
Browser tests found a bug in the event group creation from admin site.
nikomakela
nikomakela requested changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@nikomakela nikomakela left a comment

Choose a reason for hiding this comment

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

There seems to be a built-in way to handle the enum-values! I'll try this in a new PR, so we could get rid of our custom code, that seems to work OK, but brins some custom duplicated boilerplate code that we don't need to have under maintenance.

common/utils.py
Comment on lines +18 to +50
def is_enum_value(value):
"""
Check if a value is an enum value, e.g. TestEnum.FI
where TestEnum derives from enum.Enum or graphene.Enum.
"""
# Works both for enum.Enum and graphene.Enum
return type(type(value)) is enum.EnumMeta


def deepfix_enum_values(data):
"""
Fix enum values recursively in/out of dictionaries, lists, sets, and tuples.
"""
if isinstance(data, dict):
return {deepfix_enum_values(k): deepfix_enum_values(v) for k, v in data.items()}
elif isinstance(data, (list, set, tuple)):
return type(data)(deepfix_enum_values(v) for v in data)
elif is_enum_value(data):
return data.value
else:
return data


def map_enums_to_values_in_kwargs(method):
"""
Decorator that maps enums to their values in keyword arguments.
"""

def wrapper(*args, **kwargs):
fixed_kwargs = deepfix_enum_values(kwargs)
return method(*args, **fixed_kwargs)

return wrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

@karisal-anders did you test this feature https://docs.graphene-python.org/projects/django/en/latest/queries/#choices-to-enum-conversion? What about setting the convert_choices_to_enum = False?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karisal-anders did you test this feature https://docs.graphene-python.org/projects/django/en/latest/queries/#choices-to-enum-conversion? What about setting the convert_choices_to_enum = False?

No, I didn't come across that feature. If it works let's use that instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

It did not work right just how was expected, because it still had issues with the uppercase formatting. While the uppercase formatting is still an issue, it does not make sense to stick with the string inputs, when the enums offers better typing and hinting. I created a new PR of the changes #399. We can take that in action later if needed or wanted.

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://kukkuu-pr390.api.dev.hel.ninja 😆🎉🎉🎉

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://kukkuu-pr390.api.dev.hel.ninja 😆🎉🎉🎉

@nikomakela nikomakela force-pushed the KK-1108-upgrade-django-4-2 branch from 8740ffc to 2084067 Compare August 22, 2024 19:17
@nikomakela nikomakela mentioned this pull request Aug 22, 2024
Open
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@nikomakela nikomakela self-requested a review August 22, 2024 19:25
nikomakela
nikomakela approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@nikomakela nikomakela left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All the checks are now green -- even the browser tests! 👍
The schema updates are City-of-Helsinki/kukkuu-admin#276 and City-of-Helsinki/kukkuu-ui#581.

There is an alternative approach for this: #399 and City-of-Helsinki/kukkuu-admin#277 uses the enums as preferred.

@terovirtanen
Copy link
Contributor

KUKKUU-API branch is deployed to platta: https://kukkuu-pr390.api.dev.hel.ninja 🚀🚀🚀

@terovirtanen
Copy link
Contributor

TestCafe result is success for https://kukkuu-pr390.api.dev.hel.ninja 😆🎉🎉🎉

@nikomakela nikomakela merged commit e8879aa into master Aug 23, 2024
23 of 24 checks passed
@nikomakela nikomakela deleted the KK-1108-upgrade-django-4-2 branch August 23, 2024 07:25
@karisal-anders karisal-anders mentioned this pull request Oct 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomakela nikomakela nikomakela approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karisal-anders @terovirtanen @nikomakela