refactor(indexers): extract test logic into search providers#490
refactor(indexers): extract test logic into search providers#490lzinga wants to merge 4 commits intoListenarrs:canaryfrom
Conversation
|
@therobbiedavis This will conflict with #465 so it should be merged first, I can easilly solve those conflicts there @lzinga For simplicity sake, could you maybe squash the commits ? Not sure what's the politic on that topic though, I usualy do one PR, one commit except for some specific cases, also, it may be easier to wait for Robbie's review too before squashing |
…dation - Move AdditionalSettings parsing to IndexerSettingsExtensions (Try-pattern) - Add OutboundRequestSecurity validation to MAM TestAsync and SearchAsync - Consolidate IA collection parsing to use shared extension
The PR should have a squash and merge option when it gets approved. |
therobbiedavis
left a comment
therobbiedavis
left a comment
There was a problem hiding this comment.
Thanks for this! Please review my review notes
| { | ||
| private class StubHandler : HttpMessageHandler | ||
| { | ||
| private readonly HttpResponseMessage _response; |
There was a problem hiding this comment.
Should probably use a factory for the httpresponsemessage so it gets a fresh instance in case there are multiple calls and the initial call has already been disposed.
| { | ||
| return await ExecuteTestAsync(indexer); | ||
| } | ||
| catch (Exception ex) when (ex is HttpRequestException |
There was a problem hiding this comment.
This is so broad and could throw errors for unrelated bugs. What about propagate those to other handlers and use a dedicated exception type for provider validation failures?
|
|
||
| using (response) | ||
| { | ||
| response.EnsureSuccessStatusCode(); |
There was a problem hiding this comment.
So the IA provider has a logic inconsistency from the other two providers. This works, but the other providers return a IndexerTestResult.Failed() explicitly while this uses exceptions as a control flow. Can we refactor this to be more in line with the other providers for consistency?
| if (string.IsNullOrWhiteSpace(indexer.ApiKey)) | ||
| { | ||
| return IndexerTestResult.Failed( | ||
| "API key is required for Newznab/Torznab indexers", |
There was a problem hiding this comment.
Message is meant to be the user-facing summary; Error the technical detail. Passing the same string to both negates the distinction it was designed to carry. We should change this to something like:
"API key required",
"Newznab/Torznab indexers require an API key. Check your indexer settings");```
| var provider = _indexerProviders.FirstOrDefault(p => | ||
| string.Equals(p.IndexerType, indexer.Implementation, StringComparison.OrdinalIgnoreCase)) | ||
| ?? _indexerProviders.FirstOrDefault(p => | ||
| string.Equals(p.IndexerType, "Torznab", StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
This causes a behavior regression for custom indexers where it now uses the torznabnewsnabsearchprovider instead of the testgenericindexer, however I think we should just remove the option for a custom indexer from the codebase which would make this a moot issue.
3 participants
Summary
Extract indexer-specific test logic out of
IndexersControllerand into the individualIIndexerSearchProviderimplementations, so each provider owns both its search and connection-test behaviour. Closes #472.Changes
Added
IndexerProviderBaseandIndexerTestResultfor shared test infrastructureIndexerProviderTestAsyncTestscovering all three provider typesChanged
IIndexerSearchProvidernow owns itsTestAsyncimplementationIndexersControllerdelegates to providers instead of containing inline test logic (~500 lines removed)Testing
IndexerProviderTestAsyncTestscovers success and failure paths for each provider typeIndexersAuthTests,IndexersControllerTests,IndexersNewznabAuthTests,IndexersNewznabParsingTests,IndexersPersistedAuthTests, andIndexersControllerProwlarrImportTestsupdated and passingNotes
IIndexerSearchProviderclass (search + test in one place)IndexerProviderBase.TestAsyncwrapper catches common HTTP/JSON/URI exceptions so individual providers only need to implement the happy path inExecuteTestAsync