Skip to content

FIX: Database fix for empty category name - move childs and rebuild paths#216

Open
Root-Core wants to merge 1 commit intonzbdav-dev:mainfrom
Root-Core:refactor_empty_string
Open

FIX: Database fix for empty category name - move childs and rebuild paths#216
Root-Core wants to merge 1 commit intonzbdav-dev:mainfrom
Root-Core:refactor_empty_string

Conversation

@Root-Core
Copy link
Contributor

@Root-Core Root-Core commented Dec 19, 2025

Fixes #191

The current code base now handles empty categories, but it does not fix existing issues.
This PR fixes the DB and makes content accessible again.

Disclaimer: No AI was used in this pull request.

@Root-Core
Copy link
Contributor Author

Root-Core commented Dec 20, 2025

I tested it, and setting the 'cat' query parameter to an empty string does indeed cause #191, which is fixed by this PR.
I suppose the database should also be updated/repaired.

@Root-Core
Copy link
Contributor Author

This db migration should fix the database entries. I hope I haven't missed anything.

@Root-Core Root-Core changed the title Refactored empty string handling FIX: prevent empty category name and repair database entries Dec 21, 2025
public static string? GetQueryParam(this HttpContext httpContext, string name)
{
return httpContext.Request.Query[name].FirstOrDefault();
return httpContext.Request.Query[name].FirstOrDefault().OrNull();
Copy link
Owner

Choose a reason for hiding this comment

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

This change is really the only change needed to fix #191. All the other refactors in ConfigManager, HealthCheckService, DavDatabaseContext, EnvironmentUtil, StringUtil, are unecessary, imo.

return StringUtil.EmptyToNull(httpContext.Request.Query[name].FirstOrDefault());

(And I'm a fan of the db-migration to cleanup any unhealthy cases, we can keep that too)

Comment on lines +20 to +24
SELECT d.Name
FROM ConfigItems c
JOIN DavItems d
ON d.Name = c.ConfigValue
WHERE c.ConfigName = 'api.manual-category'
Copy link
Owner

Choose a reason for hiding this comment

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

Why not

SELECT c.ConfigValue
WHERE c.ConfigName = 'api.manual-category'

Comment on lines +40 to +60
migrationBuilder.Sql(@"
UPDATE OR IGNORE DavItems
SET ParentId = COALESCE(
(
SELECT Id
FROM DavItems
WHERE Name = COALESCE(
(SELECT ConfigValue FROM ConfigItems WHERE ConfigName = 'api.manual-category'),
'uncategorized'
)
),
'00000000-0000-0000-0000-000000000002'
)
WHERE ParentId = (SELECT Id FROM DavItems WHERE Name = '');
");

// Remove empty parent
// Previous duplicates will be removed due to 'ON DELETE CASCADE'
migrationBuilder.Sql(@"
DELETE FROM DavItems WHERE Name = ''
");
Copy link
Owner

Choose a reason for hiding this comment

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

I think we want,

migrationBuilder.Sql(@"
  UPDATE DavItems
  SET Name = COALESCE(
      (SELECT ConfigValue FROM ConfigItems WHERE ConfigName = 'api.manual-category'),
      'uncategorized'
  )
  WHERE Name = ''
    AND ParentId = '00000000-0000-0000-0000-000000000002'
");

This will rename the empty-name category folder to whatever is configured in the api.manual-category setting.
Then BuildFullPath should rebuild all the paths.


#nullable disable

namespace NzbWebDAV.Database.Migrations
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if this migration is worth the complexity.
I'm worried about edge cases that could end up making things worse for someone's db.

Would replacing the entire Up()` method in this migration with something like this be a safer bet?

delete from QueueItems where Category = '';
delete from HistoryItems where Category ='';
delete from DavItems where Name = '' and ParentId = '00000000-0000-0000-0000-000000000002';

This has very predictable results, and would "fix" the problem well-enough for those that encountered the #191 bug.

But if you think it's worth trying to recover those cases where prowlarr imported with empty category (instead of simply deleting them), I added some additional comments below.

@Root-Core Root-Core force-pushed the refactor_empty_string branch from 3145f83 to 7897608 Compare January 11, 2026 20:41
@Root-Core Root-Core mentioned this pull request Jan 11, 2026
@Root-Core Root-Core force-pushed the refactor_empty_string branch from 7897608 to 83dd8ba Compare January 15, 2026 01:38
@Root-Core
Copy link
Contributor Author

@nzbdav-dev Should I rebase on the dev branch or wait until you merge it into main?

I see, that you also addressed GetVariable() issue. Have a look at my solution and let me know which one you prefer.
My is a bit less verbose and more generic, yours involve less refactoring.

@Root-Core Root-Core force-pushed the refactor_empty_string branch from 83dd8ba to bb49f52 Compare January 21, 2026 02:31
@Root-Core
Copy link
Contributor Author

Root-Core commented Jan 21, 2026

Rebased and dropped commit 83dd8ba

I think this is the best of both worlds!

@Root-Core Root-Core force-pushed the refactor_empty_string branch from bb49f52 to 788218c Compare January 21, 2026 02:40
@nzbdav-dev
Copy link
Owner

Thanks for rebasing @Root-Core!

At this stage of the project, I'm inclined to keep pr acceptance to those that are minimal and single-purpose

@Root-Core Root-Core force-pushed the refactor_empty_string branch from 788218c to c7c9742 Compare January 21, 2026 03:24
@Root-Core
Copy link
Contributor Author

Root-Core commented Jan 21, 2026

  • While I appreciate the refactor, I personally think StringUtil.EmptyToNull(...) is a little more clear on what it does versus the .OrNull() extension.

One last try. :)
I renamed it to ToNullIfEmpty(), as it casts to null. This is more clear and the To part indicates the type change.

  • And I'm generally a fan of extension methods :) The repo is full of them.

You are right, I missed that.. I moved the extension into the existing StringExtensions.cs.

At this stage of the project, I'm inclined to keep pr acceptance to those that are minimal and single-purpose

I understand your point and normally I would try keeping it lean, but the PR changed a bit in it's goals.
See my replies to your comments. I think the PR is in a good state and the migration is pretty polished.

@Root-Core Root-Core force-pushed the refactor_empty_string branch from c7c9742 to 3289e97 Compare January 23, 2026 16:25
@Root-Core Root-Core force-pushed the refactor_empty_string branch from 3289e97 to 20d91a4 Compare January 24, 2026 14:41
@Root-Core Root-Core changed the title FIX: prevent empty category name and repair database entries FIX: Database fix for empty category name - move childs and rebuild paths Jan 24, 2026
@Root-Core
Copy link
Contributor Author

At this stage of the project, I'm inclined to keep pr acceptance to those that are minimal and single-purpose

I have split the refactoring out into #272, which should make things easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Missing category results in "Application Error"

2 participants