Skip to content

Members: Fix empty-string default constraints on cmsMember - #24358

Merged
AndyButland merged 3 commits into
umbraco:v17/devfrom
nltgpeterskoglund:v18/bugfix/member-empty-string-defaults
Nov 14, 2026
Merged

AndyButland merged 3 commits into
umbraco:v17/devfrom
nltgpeterskoglund:v18/bugfix/member-empty-string-defaults

Conversation

@nltgpeterskoglund

Copy link
Copy Markdown
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Description

MemberDto declares Email, LoginName and Password with [Constraint(Default = "''")]. FormatDefaultValue passes the value through GetQuotedValue, which wraps it in quotes again, so the constraint is created as:

DEFAULT ('''')   -- a single apostrophe, not an empty string

You can see this with SELECT name, definition FROM sys.default_constraints WHERE parent_object_id = OBJECT_ID('cmsMember') on a fresh SQL Server install.

This PR:

  • changes the three attributes to Default = "", so fresh installs get DEFAULT ('')
  • adds an 18.5.0 migration, FixMemberEmptyStringDefaults, which recreates the three constraints on SQL Server under their conventional DF_cmsMember_* names. It follows SetDateDefaultsToUtcNow and handles both system-generated and missing constraints. SQLite is a no-op: altering a default there requires a table rebuild, and members always save explicit values for these columns
  • adds integration tests for the migration.

GetQuotedValue itself is left unchanged, to keep this PR minimal. Escaping ' there would be a broader change for a separate discussion.

This PR and #24331 both add an entry at the end of UmbracoPlan under // To 18.5.0, so whichever is merged second needs a trivial rebase.

How to test

  1. Run the new tests on SQL Server:
    $env:Tests__Database__DatabaseType = 'LocalDb'
    dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~FixMemberEmptyStringDefaultsTests"
    
    • Fresh_Install_Has_Empty_String_Defaults fails without the DTO change ("('''')") and passes with it.
    • Can_Migrate_Single_Apostrophe_Defaults, …_System_Generated_Names and …_Are_Missing cover the upgrade paths.
  2. On SQLite (the default) the tests are ignored, because the migration is a no-op there.
  3. SchemaValidationTest, MemberRepositoryTest and MemberServiceTests still pass on LocalDB.

Local results:

  • SQLite: 1 passed, 4 ignored.
  • LocalDB (SQL Server 2025): SchemaValidationTest, MemberRepositoryTest and MemberServiceTests all passed. The new fixture's own arrange assertion was fixed and its 4 tests were re-run green.
  • The red check was confirmed by reverting the DTO change.

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown

Hi there @nltgpeterskoglund, thank you for this contribution! 👍

While we wait for someone to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

  • It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
  • The automated tests all pass (see "Checks" tab on this PR)
  • The level of security for this contribution is the same or improved
  • The level of performance for this contribution is the same or improved
  • Avoids introducing unwanted changes; note that behavioral changes might also be perceived as breaking
  • Have a look at what to check before you start contributing; if this is a larger PR, it's best to discuss it with Umbraco HQ first
  • 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@nltgpeterskoglund
nltgpeterskoglund marked this pull request as ready for review November 9, 2026 14:47
Copilot AI balanced review requested due to automatic review settings November 9, 2026 14:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The default-constraint lookup is not schema-scoped, so it can select another table’s constraint and block the upgrade; scope it to the target table’s object ID and add a two-schema regression test.

1 open issue
What changed in this PR

This PR corrects cmsMember defaults so fresh installs use empty strings rather than a single apostrophe, and repairs existing SQL Server databases during upgrade.

Changes:

  • Corrects the Email, LoginName, and Password defaults in MemberDto.
  • Registers an 18.5.0 migration to recreate SQL Server constraints; SQLite is unchanged.
  • Adds integration tests covering fresh installs and existing, system-named, or missing constraints.
File Description
tests/​Umbraco.Tests.Integration/​Umbraco.Infrastructure/​Migrations/​Upgrade/​V_18_5_0/​FixMemberEmptyStringDefaultsTests.cs Tests fresh-install defaults and migration scenarios.
src/​Umbraco.Infrastructure/​Persistence/​Dtos/​MemberDto.cs Sets the three column defaults to empty strings.
src/​Umbraco.Infrastructure/​Migrations/​Upgrade/​V_18_5_0/​FixMemberEmptyStringDefaults.cs Recreates SQL Server default constraints with corrected values.
src/​Umbraco.Infrastructure/​Migrations/​Upgrade/​UmbracoPlan.cs Adds the migration to the 18.5.0 upgrade chain.

Review effort: Balanced

@AndyButland

Copy link
Copy Markdown
Contributor

I'd be minded to simplify this @nltgpeterskoglund, and fix just the issue with the DTO so new installs get correct defaults. In practice it makes little sense to create a member without any of these fields, and we always provide them explicitly in Umbraco as you note, so these defaults never take effect. Your migration code looks good, but there may be edge cases on specific installs, and we'd have to handle it for 18, 19 and perhaps 17 too, with the complication of handling different upgrade paths.

What do you think?

@AndyButland
AndyButland force-pushed the v18/bugfix/member-empty-string-defaults branch from cd0f874 to 6b727e9 Compare November 13, 2026 14:00
@AndyButland
AndyButland changed the base branch from main to v17/dev November 13, 2026 14:00
@AndyButland AndyButland changed the title Members: Fix empty-string default constraints on cmsMember Members: Fix empty-string default constraints on cmsMember Nov 13, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@AndyButland

Copy link
Copy Markdown
Contributor

As I hadn't heard back @nltgpeterskoglund, I've gone ahead and simplified this along the lines of my earlier comment, so it can go into the next release:

  • I retargeted it to v17/dev, and rebuilt the branch on top of that, with your DTO change as its own commit under your name. It'll then merge up to the newer versions.
  • I removed migration and its tests. This also resolves the schema-scoping point Copilot raised in the migration, so I'll close that off.

Thanks again for tracking this down. Your migration was well put together, it just didn't seem worth the upgrade risk for defaults that are never used.

@AndyButland
AndyButland merged commit e2bf949 into umbraco:v17/dev Nov 14, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants