Skip to content

✨ Quality: Potential null reference when accessing Author property in AuthorCatalogFetchResult#441

Closed
halinhtvn3a wants to merge 1 commit intoListenarrs:canaryfrom
halinhtvn3a:contribai/improve/quality/potential-null-reference-when-accessing-
Closed

✨ Quality: Potential null reference when accessing Author property in AuthorCatalogFetchResult#441
halinhtvn3a wants to merge 1 commit intoListenarrs:canaryfrom
halinhtvn3a:contribai/improve/quality/potential-null-reference-when-accessing-

Conversation

@halinhtvn3a
Copy link
Copy Markdown

✨ Code Quality

Problem

The AuthorCatalogFetchResult.Author property is initialized with 'new()' but the AuthorLookupItem type is not shown. If AuthorLookupItem has non-nullable reference types that aren't properly initialized, accessing them could cause null reference exceptions. This is a data model that flows through the application and could crash at runtime.

Severity: medium
File: listenarr.api/Services/IAuthorCatalogService.cs

Solution

// Option 1: Make Author nullable if it can be null
public AuthorLookupItem? Author { get; set; }

// Option 2: Initialize with all required properties if AuthorLookupItem is defined elsewhere
// Ensure AuthorLookupItem constructor initializes all non-nullable properties

Changes

  • listenarr.api/Services/IAuthorCatalogService.cs (modified)

Testing

  • Existing tests pass
  • Manual review completed
  • No new warnings/errors introduced


🤖 About this PR

This pull request was generated by ContribAI, an AI agent
that helps improve open source projects. The change was:

  1. Discovered by automated code analysis
  2. Generated by AI with context-aware code generation
  3. Self-reviewed by AI quality checks

If you have questions or feedback about this PR, please comment below.
We appreciate your time reviewing this contribution!

Closes #440

…authorcatalogfetchresult

The AuthorCatalogFetchResult.Author property is initialized with 'new()' but the AuthorLookupItem type is not shown. If AuthorLookupItem has non-nullable reference types that aren't properly initialized, accessing them could cause null reference exceptions. This is a data model that flows through the application and could crash at runtime.

Affected files: IAuthorCatalogService.cs

Signed-off-by: halinhtvn3a <77691576+halinhtvn3a@users.noreply.github.com>
@therobbiedavis
Copy link
Copy Markdown
Collaborator

Thanks a lot for the contribution, and for taking the time to look through this area.

I reviewed the change closely, and I don’t think we should take this one as-is.

The main reason is that this change makes AuthorCatalogFetchResult.Author nullable, but it doesn’t actually fix a real unsafe path in the current implementation. AuthorLookupItem itself already only contains nullable properties, so the existing new() default is not introducing invalid non-nullable state on its own.

More importantly, the current contract in this area is effectively:

  • if author catalog lookup fails, we return null
  • if author catalog lookup succeeds, we return a result with Author populated

That pattern is relied on by existing callers, and the service implementation already follows it. So making Author nullable here would mostly just weaken the model contract and push extra null-handling into downstream code without improving runtime safety.

In other words, this looks more like a static-analysis-driven type loosening than a fix for an actual bug. If we ever find a real path where Author can be missing in a successful catalog result, I’d rather address that at the source and keep the result shape strict.

Really appreciate the submission and the effort here either way. Please keep them coming.

@therobbiedavis
Copy link
Copy Markdown
Collaborator

Closing this one out for now since we don't want to weaken the AuthorCatalogFetchResult contract without evidence of a real null-path bug.

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.

fix: potential null reference when accessing author property in authorcatalogfetchresult

2 participants