Remove a duplicate member from an enum #835
Conversation
📝 WalkthroughWalkthroughThis update introduces a migration script to correct duplicate enum values in the PostgreSQL Changes
Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
src/Altinn.Notifications.Persistence/Migration/v0.43/01-functions-and-procedures.sql (14)
1-7: Clarify autogenerated source
The header warns “Do not edit manually.” It would be helpful to reference the DbTools configuration or a README so future maintainers know how to regenerate or adjust these functions.
3-64: Streamline empty-result logic incancelorder
MixingRETURN;andRETURN QUERYcan be confusing. Instead, for the “no order” and “already cancelled” branches, you can use a singleRETURN QUERYwith aWHERE FALSEorSELECT … LIMIT 0to uniformly return an empty set or a default set.
68-85: Cache resource-limit lookup for performance
getemails_statusnew_updatestatusfetchesemaillimittimeouteach invocation. If this function is invoked at high frequency, consider caching the latest timestamp in a lightweight table or materialized view to reduce repeated scans ofresourcelimitlog.
143-170: Clarify “no rows” handling in summary functions
Bothgetemailsummary_v2andgetsmssummary_v2useRETURN QUERYthenIF NOT FOUND …. While correct, it relies onFOUNDbeing set by the precedingRETURN QUERY. For readability, you might explicitlyPERFORMa lookup and branch:PERFORM 1 FROM notifications.emailnotifications n JOIN notifications.orders o ON …; IF NOT FOUND THEN RETURN QUERY SELECT … DEFAULT ROW …; ELSE RETURN QUERY SELECT … ACTUAL ROWS …; END IF;
175-204: Consolidate metric-aggregation logic
getmetricsand the counting logic ingetorder_includestatus_v4both compute counts of sent/succeeded notifications. Extract this into a helper view or function (e.g.,notifications.notification_counts(orderid)) to avoid duplication and keep metrics consistent.
286-345: Guard JSONB parsing inget_orders_chain_tracking
You check for array-ness onReminders, but the nestedSELECT jsonb_array_elements(...)can still error if the JSON is malformed. Pre-validate with:IF jsonb_typeof(orders_chain.orderchain->'Reminders') = 'array' THEN -- safe to unnest ELSE -- return empty array END IF;This prevents runtime exceptions in edge cases.
369-385: Parameterize batch size in status update
getorders_pastsendtime_updatestatususes a hard-codedLIMIT 50. To make the migration more flexible, expose this as a function argument (e.g.,_batch_size integer) with a default, so you can tune without code changes.
388-452: Abstract multi-channel tracking
get_shipment_trackingunions order, email, and SMS tracking. If more channels (e.g., push, in-app) are added, this function will grow unwieldy. Consider a table-driven “channel registry” or view-per-channel pattern to automatically assemble tracking rows.
465-483: Unify naming in SMS recipient fetch
Ingetsmsrecipients_v2, you take a parameter named_orderidbut then alias it to__orderid. For consistency withgetemailrecipients_v2, consider naming the input_alternateidand the internal_orderid.
485-510: Schedule deprecation for backward-compat function
The comment ongetsms_statusnew_updatestatus()notes future removal, but no timeline is given. Add a target version or date to the comment so teams know when to migrate to the new function.
512-554: DRY up SMS status update overloads
The two variants ofGETSMS_STATUSNEW_UPDATESTATUSshare mostly the same logic. Move the shared query into a private helper function and have both overloads call it, passing the policy parameter.
594-642: Handle unique-key conflicts on inserts
insertemailnotification(and analogously for SMS) unconditionally inserts a new row. If the same_alternateidis retried, it will error. Consider adding anON CONFLICT (alternateid) DO NOTHINGor an explicit check to avoid runtime exceptions.
654-694: Unifyinsertorderoverloads via default args
PostgreSQL 14+ supports default values on function parameters. You can define a singleinsertorderfunction with_sendingtimepolicy integer DEFAULT NULLto avoid duplication.
778-787: Consider logging or auditing status updates
updateemailstatussilently updates theemailnotificationstable. For operational observability, you may want to emit aRAISE NOTICEor insert into an audit table when important statuses change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Notifications.Persistence/Migration/v0.43/01-alter-types.sql(1 hunks)src/Altinn.Notifications.Persistence/Migration/v0.43/01-functions-and-procedures.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: Build, test & analyze
🔇 Additional comments (6)
src/Altinn.Notifications.Persistence/Migration/v0.43/01-alter-types.sql (3)
9-29: Ensure correct enum namespace and idempotency
The script checks forsmsnotificationresulttype_newin thepublicnamespace but your production type may reside elsewhere (e.g., innotifications). Confirm thatsmsnotificationresulttypelives inpublic. To avoid collisions with identically named types in other schemas, include a namespace filter when queryingpg_type:WHERE typname = 'smsnotificationresulttype_new' AND typnamespace = 'public'::regnamespace
33-43: Minimize locking and hard-code safety in data migration
AnACCESS EXCLUSIVElock will block all DML onnotifications.smsnotifications. Validate that this maintenance window is acceptable. Also, theUSINGclause only catches one specific casing. To guard against any case variation, consider normalizing withLOWER:- WHEN result::text = 'Failed_Recipientreserved' THEN 'Failed_RecipientReserved' + WHEN LOWER(result::text) = 'failed_recipientreserved' THEN 'Failed_RecipientReserved'
82-89: Validate drop/rename ordering and residual types
After dropping the old type and renaming the new one, confirm that no stray types with_newsuffix remain. You can add a verification step:-- Ensure no lingering "_new" types SELECT n.nspname, t.typname FROM pg_type t JOIN pg_namespace n ON t.typnamespace = n.oid WHERE t.typname LIKE '%_new';This guards against orphaned types before you proceed with downstream deployments.
src/Altinn.Notifications.Persistence/Migration/v0.43/01-functions-and-procedures.sql (3)
697-726: Revisit PROCEDURE vs FUNCTION choice
You're mixingPROCEDUREs (no return) andFUNCTIONs. If you ever need to compose the inserts into larger SELECT statements,FUNCTIONs are required. Confirm this split is intentional.
728-775: Ensure SMS insert aligns with updated enum values
insertsmsnotificationcasts_result::smsnotificationresulttype. Any mismatch between the incoming_resultstring and your new enum will fail. Confirm your application layer only supplies valid enum labels.
562-593:❓ Verification inconclusive
Verify enum-cast compatibility post-migration
Several functions cast tosmsnotificationresulttypeorsmsnotificationresulttype_new. After running your migration, ensure that PostgreSQL correctly resolves the casts without errors. You can automate a quick check:
🏁 Script executed:
#!/bin/bash psql -d mydb -c "SELECT pg_typeof('New'::smsnotificationresulttype);"Length of output: 172
Verify enum-cast compatibility post-migration
After applying your SQL migration, please confirm that PostgreSQL can resolve casts to both the old and new enum types without errors. In your target database environment, run:psql -d <your_database> -c "SELECT pg_typeof('New'::smsnotificationresulttype);" psql -d <your_database> -c "SELECT pg_typeof('New'::smsnotificationresulttype_new);"If either command fails, update the enum definitions or migration scripts accordingly to ensure downstream functions (e.g., getsmssummary_v2) cast cleanly.
| DO $$ | ||
| DECLARE | ||
| rec RECORD; | ||
| def TEXT; | ||
| BEGIN | ||
| FOR rec IN | ||
| SELECT n.nspname, c.relname, c.relkind | ||
| FROM pg_depend d | ||
| JOIN pg_type t ON d.objid = t.oid | ||
| JOIN pg_class c ON d.refobjid = c.oid | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| WHERE t.typname = 'smsnotificationresulttype' AND c.relkind IN ('v','m') | ||
| LOOP | ||
| def := pg_get_viewdef(format('%I.%I', rec.nspname, rec.relname), true); | ||
| IF rec.relkind = 'v' THEN | ||
| EXECUTE format('CREATE OR REPLACE VIEW %I.%I AS %s', rec.nspname, rec.relname, def); | ||
| ELSE | ||
| EXECUTE format('CREATE OR REPLACE MATERIALIZED VIEW %I.%I AS %s', rec.nspname, rec.relname, def); | ||
| EXECUTE format('REFRESH MATERIALIZED VIEW %I.%I', rec.nspname, rec.relname); | ||
| END IF; | ||
| END LOOP; | ||
|
|
||
| FOR rec IN | ||
| SELECT n.nspname, p.proname, p.oid | ||
| FROM pg_depend d | ||
| JOIN pg_type t ON d.objid = t.oid | ||
| JOIN pg_proc p ON d.refobjid = p.oid | ||
| JOIN pg_namespace n ON n.oid = p.pronamespace | ||
| WHERE t.typname = 'smsnotificationresulttype' | ||
| LOOP | ||
| def := pg_get_functiondef(rec.oid); | ||
| def := regexp_replace(def, '\m smsnotificationresulttype \M', 'smsnotificationresulttype_new', 'g'); | ||
| EXECUTE def; | ||
| END LOOP; | ||
| END | ||
| $$; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden dependent-object recreation and regex replacement
- The regex
'\m smsnotificationresulttype \M'may miss schema-qualified names (e.g.public.smsnotificationresulttype). - To catch both unqualified and qualified references, use:
- def := regexp_replace(def, '\m smsnotificationresulttype \M', 'smsnotificationresulttype_new', 'g');
+ def := regexp_replace(def,
+ '\m(\w+\.)?smsnotificationresulttype\M',
+ 'public.smsnotificationresulttype_new',
+ 'g');- Consider filtering
pg_dependont.typnamespaceto only rebuild objects in the intended schema.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DO $$ | |
| DECLARE | |
| rec RECORD; | |
| def TEXT; | |
| BEGIN | |
| FOR rec IN | |
| SELECT n.nspname, c.relname, c.relkind | |
| FROM pg_depend d | |
| JOIN pg_type t ON d.objid = t.oid | |
| JOIN pg_class c ON d.refobjid = c.oid | |
| JOIN pg_namespace n ON n.oid = c.relnamespace | |
| WHERE t.typname = 'smsnotificationresulttype' AND c.relkind IN ('v','m') | |
| LOOP | |
| def := pg_get_viewdef(format('%I.%I', rec.nspname, rec.relname), true); | |
| IF rec.relkind = 'v' THEN | |
| EXECUTE format('CREATE OR REPLACE VIEW %I.%I AS %s', rec.nspname, rec.relname, def); | |
| ELSE | |
| EXECUTE format('CREATE OR REPLACE MATERIALIZED VIEW %I.%I AS %s', rec.nspname, rec.relname, def); | |
| EXECUTE format('REFRESH MATERIALIZED VIEW %I.%I', rec.nspname, rec.relname); | |
| END IF; | |
| END LOOP; | |
| FOR rec IN | |
| SELECT n.nspname, p.proname, p.oid | |
| FROM pg_depend d | |
| JOIN pg_type t ON d.objid = t.oid | |
| JOIN pg_proc p ON d.refobjid = p.oid | |
| JOIN pg_namespace n ON n.oid = p.pronamespace | |
| WHERE t.typname = 'smsnotificationresulttype' | |
| LOOP | |
| def := pg_get_functiondef(rec.oid); | |
| def := regexp_replace(def, '\m smsnotificationresulttype \M', 'smsnotificationresulttype_new', 'g'); | |
| EXECUTE def; | |
| END LOOP; | |
| END | |
| $$; | |
| FOR rec IN | |
| SELECT n.nspname, p.proname, p.oid | |
| FROM pg_depend d | |
| JOIN pg_type t ON d.objid = t.oid | |
| JOIN pg_proc p ON d.refobjid = p.oid | |
| JOIN pg_namespace n ON n.oid = p.pronamespace | |
| WHERE t.typname = 'smsnotificationresulttype' | |
| LOOP | |
| def := pg_get_functiondef(rec.oid); | |
| - def := regexp_replace(def, '\m smsnotificationresulttype \M', 'smsnotificationresulttype_new', 'g'); | |
| + def := regexp_replace(def, | |
| + '\m(\w+\.)?smsnotificationresulttype\M', | |
| + 'public.smsnotificationresulttype_new', | |
| + 'g'); | |
| EXECUTE def; | |
| END LOOP; |
🤖 Prompt for AI Agents (early access)
In src/Altinn.Notifications.Persistence/Migration/v0.43/01-alter-types.sql
around lines 45 to 80, the regex used to replace 'smsnotificationresulttype' in
function definitions does not account for schema-qualified names, which can
cause missed replacements. Update the regex to match both unqualified and
schema-qualified references of the type name. Additionally, refine the pg_depend
queries by filtering on t.typnamespace to restrict dependent object recreation
to the intended schema only, preventing unintended object modifications outside
the target schema.
|
Description
These changes aim to remove a single member from an enumeration used in the database.
Related Issue(s)
Failed_RecipientreservedfromSMSNotificationResultType#810Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes