ardalis / ardalis/CleanArchitecture

⚠️ SeedData may cause unintended deletion of production data if ToDoList is not used

Open
#953 2 comments 1 reaction 0 assignees View on GitHub
Dominant language
C#
Stars
18.5k
Forks
3.1k
PR merge metrics
No merged PRs in 30d

Description

Hi and thanks for maintaining this great template, it's very useful to explore clean architecture in .NET.

I’d like to raise awareness of a potential data loss risk in SeedData.cs that could arise in some specific conditions, especially when developers use the sample as a base for real applications.

🔍 **Context**
In [SeedData.cs](https://github.com/ardalis/CleanArchitecture/blob/ec2d7fc4a62bb7e8e42a946f633644d9cf583a7b/sample/src/NimblePros.SampleToDo.Web/SeedData.cs#L38), the following logic is used:

``` csharp
public static async Task InitializeAsync(AppDbContext dbContext)
{
if (await dbContext.ToDoItems.AnyAsync()) // DANGEROUS CHECK !!!
{
return;
}

// if dbContext.ToDoItems is emptied or removed, potential risk to loose all data !!!

await PopulateTestDataAsync(dbContext);
}

public static async Task PopulateTestDataAsync(AppDbContext dbContext)
{
foreach (var item in dbContext.Projects)
{
dbContext.Remove(item);
}
foreach (var item in dbContext.ToDoItems)
{
dbContext.Remove(item);
}
foreach (var item in dbContext.Contributors)
{
dbContext.Remove(item);
}

// etc etc other production tables could be added here

await dbContext.SaveChangesAsync();
```

⚠️ Potential Risk
This behavior may cause accidental data loss in production in the following (not unrealistic) situation:
- A production application uses the template but do not remove ToDoList, while not using it, and keeping the seeding mechanism in place (for seeding other data).
- A DBA or ops team **removes or cleans** the unused ToDoLists table in production to clean up the schema.
- A local/**dev** version of the application connects to the **production** DB (by mistake or for diagnostics using prod connection string).

To note that table removal in the DB will not generate exception, as we have in the MiddlewareConfig the `EnsureCreatedAsync` so the eventually removed table is _recreated_.

```
await context.Database.EnsureCreatedAsync();
await SeedData.InitializeAsync(context);
```

So, while removing or truncating ToDoItems, the condition `!context.ToDoLists.Any()` would be true, and the deletion logic before that would wipe all data from ToDoItems, ToDoLists etc etc, even if other parts of the application used them or the model was extended.

✅ **Suggestion**
It might be safer to:

- Do not delete data.
- Or provide a warning/comment in the template that this logic is only meant for sample/dev environments and should never be deployed as-is, however, there is always a risk the developers don't real the full code or all comments.

Again, this isn’t a bug per se, and certainly not a mistake from your side, but a small design choice in the sample code that might have big consequences in derived applications if developers aren’t careful.

Thanks again for your work on this project!

Contributor guide

Open the contributing guide

Research direction

Start with sample/src/NimblePros.SampleToDo.Web/SeedData.cs, especially InitializeAsync and PopulateTestDataAsync, then read MiddlewareConfig’s EnsureCreatedAsync call. Decide on safe seeding behavior when ToDoItems is empty or recreated, and verify the startup path no longer risks deleting existing application data when the sample table is absent or unused.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.