Add a ApiKeyFile option
#511
Conversation
WalkthroughAdds ApiKeyFile support and validation across interfaces, models, V1 adapters, config loader, examples, docs, and tests. LoadConfig now delegates parsing to a new LoadConfigRaw and invokes Validate() on the parsed settings; account validation enforces mutual-exclusivity, can read ApiKey from a file, and requires one credential per account. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigLoader
participant Parser
participant ServerSettings
participant Account
participant FileSystem
Client->>ConfigLoader: LoadConfig(path)
ConfigLoader->>Parser: LoadConfigRaw(path)
Parser-->>ConfigLoader: IServerSettings (parsed)
ConfigLoader->>ServerSettings: Validate()
ServerSettings->>ServerSettings: GeneralSettings.Validate()
loop for each Account
ServerSettings->>Account: ValidateAndInitialize()
alt ApiKeyFile provided
Account->>FileSystem: read(ApiKeyFile)
FileSystem-->>Account: file contents
Account->>Account: ApiKey = trimmed(contents)
else Both ApiKey and ApiKeyFile provided
Account-->>ServerSettings: throw validation error
else No credential provided
Account-->>ServerSettings: throw validation error
end
end
ServerSettings-->>ConfigLoader: validated settings
ConfigLoader-->>Client: IServerSettings (validated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(1 hunks)docs/docs/getting-started/configuration.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ImmichFrame.WebApi/Models/ServerSettings.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
VerifyConfig(71-75)
ImmichFrame.Core/Interfaces/IServerSettings.cs (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-105)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-105)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-105)
🔇 Additional comments (6)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
23-28: LGTM! Clean separation of loading and validation.The introduction of a public
LoadConfigmethod that wrapsLoadConfigRawand ensures validation is performed is a clean design pattern. This guarantees that all loaded configurations are validated before use.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (2)
66-66: LGTM! Appropriate no-op validation for V1 compatibility.The no-op
validate()implementations in the V1 adapters appropriately satisfy the interface contracts without changing V1 behavior, maintaining backward compatibility.
72-72: LGTM! Correct default for unsupported V1 feature.Returning an empty string for
ApiKeyFilecorrectly indicates that V1 settings didn't support this feature.ImmichFrame.Core/Interfaces/IServerSettings.cs (2)
8-8: LGTM! Interface extensions support validation flow.The addition of
validate()methods to the interfaces enables consistent post-load validation across all configuration layers.
15-15: LGTM! ApiKeyFile property added to support the new feature.The
ApiKeyFileproperty extension toIAccountSettingscleanly supports the new API key file configuration option.ImmichFrame.WebApi/Models/ServerSettings.cs (1)
27-35: LGTM! Validation properly cascades through the configuration hierarchy.The
validate()implementation correctly propagates validation to bothGeneralSettingsand all account configurations, ensuring comprehensive validation.
jfly
force-pushed
the
issue-510-add-ApiKeyFile
branch
2 times, most recently
from
06b6ced to
82e6324
Compare
|
Tests are failing for this. Could you fix the tests (add your new option to the base test files)? Also adding docs for this would be great! |
jfly
force-pushed
the
issue-510-add-ApiKeyFile
branch
from
82e6324 to
2594818
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ImmichFrame.WebApi.Tests/Resources/TestV2.yml (1)
38-53: TestV2.yml sets both ApiKey and ApiKeyFile, conflicting with validation rules
ServerAccountSettings.validate()enforces that exactly one ofApiKeyorApiKeyFileis provided and, whenApiKeyFileis set, attempts to read the key from that file. In this fixture the first account has both fields populated, andApiKeyFilepoints to a test string that is unlikely to exist as a real file. Any test or code path that loads this viaConfigLoader.LoadConfig(which now always callsconfig.validate()) will see validation/file‑IO failures, and even whenvalidate()isn’t called this fixture no longer models a valid configuration.If the intent is for this file to represent a valid V2 config, consider making the first account purely file‑based and relying on the second account to cover inline keys, e.g.:
- - ImmichServerUrl: Account1.ImmichServerUrl_TEST - ApiKey: Account1.ApiKey_TEST - ApiKeyFile: Account1.ApiKeyFile_TEST + - ImmichServerUrl: Account1.ImmichServerUrl_TEST + # Account1 uses file-based API key to exercise ApiKeyFile handling + ApiKeyFile: Account1.ApiKeyFile_TESTThis keeps coverage for both ApiKeyFile (account 1) and ApiKey (account 2) while aligning with the validation contract.
Also applies to: 68-68
ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
40-60: TestV2.json first account also violates ApiKey vs ApiKeyFile mutual exclusivityAs with the YAML fixture, the first account here sets both
"ApiKey"and"ApiKeyFile", which contradicts the runtime contract enforced byServerAccountSettings.validate(). GivenConfigLoader.LoadConfignow always invokesconfig.validate(), this JSON fixture can’t safely represent a “valid” V2 configuration.To align it with the model while still covering both scenarios, you can mirror the YAML change and make account 1 file‑based and account 2 inline:
- { - "ImmichServerUrl": "Account1.ImmichServerUrl_TEST", - "ApiKey": "Account1.ApiKey_TEST", - "ApiKeyFile": "Account1.ApiKeyFile_TEST", + { + "ImmichServerUrl": "Account1.ImmichServerUrl_TEST", + // Account1 uses file-based API key to exercise ApiKeyFile handling + "ApiKeyFile": "Account1.ApiKeyFile_TEST",This keeps tests exercising ApiKeyFile without conflicting with the validation behavior.
🧹 Nitpick comments (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
3-9: Clarify and align the newvalidate()surface across settings interfacesCentralizing validation via
validate()onIServerSettings,IAccountSettings, andIGeneralSettingsplus addingApiKeyFiletoIAccountSettingsmakes the contracts explicit. Two optional improvements to consider:
- Use PascalCase
Validate()to match usual .NET naming conventions for methods on interfaces.- Add XML doc comments explaining when callers are expected to invoke validation and what invariants (e.g., exactly one of
ApiKey/ApiKeyFile) are guaranteed afterward, since the interface itself can’t enforce thatvalidate()is called.These are polish items; the current shape is functionally sound.
Also applies to: 11-28, 30-67
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
23-29: Nice separation of loading vs. validation; consider normalizing validation errorsRefactoring the existing logic into
LoadConfigRawand havingLoadConfigcallconfig.validate()centralizes validation nicely. For consistency withLoadConfigJson/LoadConfigYaml, you might want to catch exceptions fromconfig.validate()here and wrap them in aSettingsNotValidExceptionso callers see a uniform exception type for “config invalid” conditions.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
61-67: V1 adapters correctly implement the new surface; optional: add minimal validationReturning
ApiKeyFile => nullforAccountSettingsV1Adapteris a good way to satisfy the new interface without changing legacy semantics, and the emptyvalidate()implementations ensure older configs still load. If you want legacy/ENV configs to get some of the same safety as V2 (e.g., non-emptyImmichServerUrlandApiKey, positive intervals), you could add lightweight checks inside thesevalidate()methods, but it’s reasonable to leave them as no-ops if V1 is truly “compatibility only.”Also applies to: 68-85, 87-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(2 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(2 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(1 hunks)docs/docs/getting-started/configuration.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docker/Settings.example.json
- ImmichFrame.WebApi/Models/ServerSettings.cs
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
🔇 Additional comments (3)
docs/docs/getting-started/configuration.md (2)
87-95: ApiKey / ApiKeyFile example now correctly reflects mutual exclusivityThe note plus using ApiKey as the active value and ApiKeyFile as a commented alternative matches the runtime validation rules and avoids showing an invalid config.
166-178: Custom CSS example and fenced block look correctThe volume mapping example and closing fence/newline are well‑formed; no further changes needed.
docker/Settings.example.yml (1)
36-40: Example config now demonstrates a valid ApiKey vs ApiKeyFile setupHaving ApiKey active with ApiKeyFile commented, plus the “Exactly one” comment, cleanly aligns the example with the validation contract.
jfly
force-pushed
the
issue-510-add-ApiKeyFile
branch
2 times, most recently
from
a6c6295 to
c72de4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
8-8: Consider using PascalCase for method names.C# conventions typically use PascalCase for public method names (
Validate()rather thanvalidate()). While this is functional, it deviates from standard .NET naming conventions.- public void validate(); + public void Validate();Apply the same change to all three
validate()declarations (lines 8, 27, and 66).Also applies to: 27-27, 66-66
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
110-117: Consider adding validation tests for mutual exclusivity.The property verification logic correctly handles ApiKeyFile assertions. However, the test suite currently only tests config loading, not config validation.
Consider adding tests that verify:
validate()succeeds when onlyApiKeyis providedvalidate()succeeds when onlyApiKeyFileis provided (with a valid file)validate()throws when both are providedvalidate()throws when neither is providedWould you like me to generate test cases for the validation logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs(5 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(3 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(1 hunks)docs/docs/getting-started/configuration.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ImmichFrame.WebApi/Models/ServerSettings.cs
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- docker/Settings.example.json
- ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs
- docker/Settings.example.yml
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
🔇 Additional comments (9)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
14-15: Note:ApiKeygetter allows mutation fromvalidate().The
ApiKeyproperty is defined with only a getter in the interface, but the implementation inServerAccountSettings.validate()assigns to it viaApiKey = File.ReadAllText(ApiKeyFile).Trim(). This works because the concrete class has a setter, but it's worth noting that the interface contract doesn't expose this mutability.If you want the interface to explicitly reflect that
ApiKeycan be set during validation, consider adding a setter or documenting this behavior. Otherwise, this is acceptable as-is since the implementation handles it correctly.ImmichFrame.WebApi.Tests/Resources/TestV2.yml (1)
38-56: Test data contains bothApiKeyandApiKeyFile, which would fail validation.Both accounts specify
ApiKeyandApiKeyFiletogether. Per the validation logic inServerAccountSettings.validate(), this configuration throws: "Cannot specify both ApiKey and ApiKeyFile."This works because
LoadConfigJson/LoadConfigYamldon't invokevalidate(), but it may cause confusion. Consider either:
- Removing
ApiKeyFilefrom test accounts (keeping onlyApiKey), or- Adding a separate test file that exercises the mutual exclusivity validation
Are there separate tests that verify the mutual exclusivity validation throws appropriately when both fields are provided?
ImmichFrame.WebApi.Tests/Resources/TestV1.json (1)
57-57: LGTM!Trailing newline added - good file formatting practice.
docs/docs/getting-started/configuration.md (1)
92-94: LGTM!The documentation correctly demonstrates the mutual exclusivity between
ApiKeyandApiKeyFileby showing one option active and the alternative commented out. The explanatory comment on line 92 is clear.ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
71-75: LGTM!The test helper correctly propagates the
expectNullApiKeyFileflag through the verification chain to handle both V1 (null ApiKeyFile) and V2 (populated ApiKeyFile) configurations.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (4)
66-66: LGTM: No-op validation appropriate for V1 adapter.The empty
validate()implementation correctly satisfies theIServerSettingsinterface requirement while maintaining backward compatibility for V1 configs. Validation logic for V2 configs is handled in the actual models (e.g.,ServerAccountSettings.validate()).
72-72: LGTM: Clear explanation for null ApiKeyFile.Returning
nullforApiKeyFileis correct since V1 configuration format didn't support file-based API keys. The inline comment clearly documents this design decision.
83-84: LGTM: Consistent with adapter pattern.The no-op
validate()implementation is appropriate for the V1 adapter, following the same pattern as the other adapter classes in this file.
122-123: LGTM: Interface requirement satisfied.The no-op
validate()implementation correctly implements theIGeneralSettingsinterface requirement while maintaining V1 backward compatibility.
Done. This unfortunately got pretty janky. The tests as written are a little too generic (assuming every single string setting's value is a straightforward modification of the setting name).
I made a very minor tweak to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
110-117: Conditional assertion correctly handles ApiKeyFile property.The logic properly asserts that ApiKeyFile is null for V1 configs and has the expected test value for V2 configs. This maintains test coverage across config versions.
Consider adding dedicated tests for the validation logic (in a separate test file for ServerSettings.validate()) to cover:
- Both ApiKey and ApiKeyFile set → exception
- Neither ApiKey nor ApiKeyFile set → exception
- ApiKeyFile points to valid file → ApiKey populated from file content
- ApiKeyFile points to missing file → helpful error message
These validation scenarios would complement the config loading tests here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs(5 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(3 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(1 hunks)docs/docs/getting-started/configuration.md(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
- ImmichFrame.WebApi/Models/ServerSettings.cs
- docker/Settings.example.json
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
🔇 Additional comments (6)
docker/Settings.example.yml (1)
38-40: LGTM! Example now demonstrates valid configuration.The mutual exclusivity between ApiKey and ApiKeyFile is clearly documented, and the example shows a valid configuration with ApiKey set and ApiKeyFile commented out. This properly addresses the previous review feedback.
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
8-8: LGTM! Clean and consistent interface extensions.The addition of
validate()methods across all three interfaces (IServerSettings, IAccountSettings, IGeneralSettings) and the nullableApiKeyFileproperty to IAccountSettings are well-designed. The consistency across interfaces and the nullable type for the optional ApiKeyFile field follow good practices.Also applies to: 15-15, 27-27, 66-66
docs/docs/getting-started/configuration.md (1)
15-15: LGTM! Documentation clearly explains the new option.The documentation updates accurately reflect the ApiKeyFile feature, clearly indicating that exactly one of ApiKey or ApiKeyFile is required. The example configuration properly demonstrates mutual exclusivity by commenting out the ApiKeyFile option.
Also applies to: 92-94
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (2)
29-88: LGTM! LoadConfigRaw preserves existing loading logic.The refactoring to extract the raw loading logic into a private method is clean and maintains all existing behavior (JSON current/legacy, YAML current/legacy, environment variables).
23-28: Good separation of loading and validation logic.The new
LoadConfigwrapper that callsLoadConfigRawand thenconfig.validate()is a clean architectural improvement. This ensures that configuration validation (including ApiKey/ApiKeyFile mutual exclusivity and file reading) is consistently enforced.However, verification of the
validate()implementation in ServerSettings.cs could not be completed due to repository access limitations. Ensure that thevalidate()method properly handles file I/O errors whenApiKeyFilepoints to a non-existent or unreadable file—specifically, theFile.ReadAllTextcall should catchIOExceptionand provide context like "ApiKeyFile '{path}' not found or could not be read."ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
32-32: LGTM! Tests properly handle V1 vs V2 ApiKeyFile expectations.The updated test calls correctly pass
expectNullApiKeyFile=truefor V1 configs (which don't support ApiKeyFile) andexpectNullApiKeyFile=falsefor V2 configs (which include ApiKeyFile). This ensures backward compatibility.Also applies to: 42-42, 50-50, 68-68
3rob3
left a comment
3rob3
left a comment
There was a problem hiding this comment.
I forgot to submit this a few weeks ago, sorry.
jfly
force-pushed
the
issue-510-add-ApiKeyFile
branch
from
c72de4d to
77453aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
11-28: Make account-level validation idempotent when usingApiKeyFileThe addition of
ApiKeyFileandvalidate()onIAccountSettingsis the right abstraction. One concern from theServerAccountSettings.validate()implementation (inImmichFrame.WebApi/Models/ServerSettings.cs) is that it reads the API key fromApiKeyFileintoApiKeybut leavesApiKeyFilenon‑null and then rejects configurations where both are set. This makesvalidate()unsafe to call more than once: a second call after a successful first call will see bothApiKeyandApiKeyFilepopulated and throw.Consider making the account-level validation idempotent, for example by clearing
ApiKeyFileafter a successful file read or restructuring the logic so that a second call with an already-loaded key does not fail. For instance:public void validate() { if (!string.IsNullOrWhiteSpace(ApiKeyFile) && !string.IsNullOrWhiteSpace(ApiKey)) { throw new Exception("Cannot specify both ApiKey and ApiKeyFile. Please provide only one."); } if (!string.IsNullOrWhiteSpace(ApiKeyFile) && string.IsNullOrWhiteSpace(ApiKey)) { ApiKey = File.ReadAllText(ApiKeyFile).Trim(); ApiKeyFile = null; // optional but makes repeated validate() calls safe } if (string.IsNullOrWhiteSpace(ApiKey)) { throw new InvalidOperationException("Either ApiKey or ApiKeyFile must be provided."); } }This keeps the mutual-exclusion semantics while avoiding surprises if
validate()is ever called more than once on the same instance.
♻️ Duplicate comments (1)
docker/Settings.example.json (1)
42-43: Configuration example violates mutual exclusivity (duplicate concern).This example still includes both
ApiKeyandApiKeyFile, which will fail validation since the codebase enforces that only one can be set. This was flagged in a previous review. Since you and others preferred not to use JSON comments, consider either:
- Remove
ApiKeyFileto show the traditional approach, or- Remove
ApiKeyto showcase the new feature.A working example is more valuable to users than demonstrating both options that cannot coexist.
🧹 Nitpick comments (4)
ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
30-67: No-opIGeneralSettings.validate()is fine for now, but document/extend later if neededAdding
validate()toIGeneralSettingsaligns with the pattern used forIServerSettingsandIAccountSettings, and the current no-op implementation is acceptable. If you later add invariants for general settings (e.g., validatingInterval,Language, or weather settings), you already have a hook to put them in.ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (2)
46-51: Consider avoiding “both ApiKey and ApiKeyFile set” in V2 sample configsFor V2 JSON/YAML,
VerifyConfig(config, true, false)still expects bothApiKeyandApiKeyFilestring properties to be populated with the..._TESTpattern. That works for the reflection-based shape tests, but it conflicts with the runtime validation rule that you can't specify both an inline API key andApiKeyFile.It may be cleaner (and less confusing for future readers) to adjust these tests so that V2 resources model one of the valid configurations (either inline key or file-based key), and add a separate, focused test that asserts the mutual-exclusion behavior via
validate(), instead of encoding “both set” into the generic sample config.Also applies to: 64-69
71-88: TargetedApiKeyFilehandling inVerify*helpers is reasonableThreading
expectNullApiKeyFilethroughVerifyConfig→VerifyAccounts→VerifyPropertiesand special-casingprop.Name == "ApiKeyFile"keeps the existing generic “property name → value pattern” assertions intact without over-complicating the test harness.The conditional:
if (prop.Name.Equals("ApiKeyFile") && expectNullApiKeyFile) { Assert.That(value, Is.EqualTo(null), prop.Name); } else { Assert.That(value, Is.EqualTo(prefix + prop.Name + "_TEST"), prop.Name); }matches the current adapter behavior for V1 vs V2 and keeps the change localized.
Also applies to: 107-117
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
61-67: V1 adapters correctly exposeApiKeyFileas unsupported and provide no-op validationHaving
AccountSettingsV1Adapter.ApiKeyFile => nulland implementing emptyvalidate()methods onServerSettingsV1Adapter,AccountSettingsV1Adapter, andGeneralSettingsV1Adapteris an appropriate way to satisfy the new interface contracts without altering legacy V1 behavior. This also aligns with the tests that expectApiKeyFileto be null for V1.If you expect to add more cross-version validation in the future, a brief comment on
ServerSettingsV1Adapter.validate()noting that V1 validation is intentionally a no-op would make the intent explicit.Also applies to: 68-85, 87-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs(5 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(3 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(1 hunks)docker/example.env(2 hunks)docs/docs/getting-started/configuration.md(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- docker/example.env
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
🚧 Files skipped from review as they are similar to previous changes (5)
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
- ImmichFrame.WebApi/Models/ServerSettings.cs
- docs/docs/getting-started/configuration.md
- docker/Settings.example.yml
🧰 Additional context used
🧬 Code graph analysis (3)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
ServerSettingsV1Adapter(61-125)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (2)
IServerSettings(23-28)IServerSettings(29-88)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
ImmichFrame.Core/Interfaces/IServerSettings.cs (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
🔇 Additional comments (3)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
23-29: Centralizing validation inLoadConfiglooks goodWrapping the existing load logic in
LoadConfigRawand callingconfig.validate()inLoadConfigis a clean separation of concerns and makes it clear where validation happens. This will ensure ApiKey/ApiKeyFile rules are enforced consistently for all config sources that go throughLoadConfig.ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
3-9: Interface-levelvalidate()makes sense for a single validation entry pointAdding
validate()toIServerSettingsprovides a clear contract for running configuration validation after loading and matches howConfigLoader.LoadConfigis now structured.ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
28-43: V1 tests correctly assertApiKeyFileis unavailablePassing
expectNullApiKeyFile: truefor V1 JSON and env paths and having the adapter exposeApiKeyFile => nullis a good way to keep the generic property assertions while signaling that V1 configs don't support file-based API keys.
If given, `ApiKeyFile` points at a file that ImmichFrame will read to get the api key. `ApiKeyFile` is mutually exclusive with `ApiKey`. This fixes immichFrame#510
jfly
force-pushed
the
issue-510-add-ApiKeyFile
branch
from
77453aa to
f5bb164
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docker/Settings.example.json (1)
41-43: JSON example still invalid: sets both ApiKey and ApiKeyFileThis example config will fail validation because
ServerAccountSettings.validate()throws when bothApiKeyandApiKeyFileare provided. Users copying this file will get an immediate error.Since JSON can’t express “either/or” with comments, the example should keep only one of the two fields so it’s a valid, runnable config.
One simple fix is to drop
ApiKeyFilehere and rely on the YAML/docs to showcase that option:"Accounts": [ { "ImmichServerUrl": "REQUIRED", - "ApiKey": "super-secret-api-key", - "ApiKeyFile": "/path/to/api.key", + "ApiKey": "super-secret-api-key", "ImagesFromDate": null,Alternatively, you could remove
ApiKeyand keep onlyApiKeyFile, but only one should remain in this JSON example.Also applies to: 62-62
🧹 Nitpick comments (1)
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
27-35: Validation flow and ApiKeyFile handling look correct; consider tightening exception type
ServerSettings.validate()correctly walksGeneralSettingsand each account, wiring validation into the load flow.ServerAccountSettings.validate()enforces:
- ApiKey/ApiKeyFile mutual exclusivity,
- reading the key from
ApiKeyFilewhen present,- and requiring an effective API key at the end.
To keep exceptions consistent and more idiomatic, you might want the mutual-exclusivity branch to throw
InvalidOperationException(like the “either ApiKey or ApiKeyFile must be provided” case) instead of a bareException, e.g.:- if (!string.IsNullOrWhiteSpace(ApiKeyFile)) + if (!string.IsNullOrWhiteSpace(ApiKeyFile)) { if (!string.IsNullOrWhiteSpace(ApiKey)) { - throw new Exception("Cannot specify both ApiKey and ApiKeyFile. Please provide only one."); + throw new InvalidOperationException("Cannot specify both ApiKey and ApiKeyFile. Please provide only one."); } ApiKey = File.ReadAllText(ApiKeyFile).Trim(); }Functionality is fine as-is; this is just a small polish.
Also applies to: 74-75, 81-81, 94-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs(5 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(3 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(1 hunks)docker/example.env(2 hunks)docs/docs/getting-started/configuration.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker/example.env
🚧 Files skipped from review as they are similar to previous changes (4)
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- docker/Settings.example.yml
🧰 Additional context used
🧬 Code graph analysis (4)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
ImmichFrame.WebApi/Models/ServerSettings.cs (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
validate(8-8)validate(27-27)validate(66-66)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
validate(66-66)validate(84-84)validate(123-123)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
validate(27-35)validate(74-74)validate(94-109)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
ServerSettingsV1Adapter(61-125)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (2)
IServerSettings(23-28)IServerSettings(29-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
docs/docs/getting-started/configuration.md (1)
30-30: ApiKey / ApiKeyFile docs and example look consistent with validation rulesThe text and YAML example now clearly express mutual exclusivity and show only one of
ApiKey/ApiKeyFileactive, which matches the runtime validation behavior.Also applies to: 107-109
ImmichFrame.WebApi.Tests/Resources/TestV2.yml (1)
40-40: Test fixture updates align with reflection-based verifierThe added
ApiKeyFileentries and People indentation match theVerifyPropertiespattern (prefix + PropName + "_TEST"), so these resources should integrate cleanly with the updated tests.Also applies to: 56-56, 69-69
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
23-29: Centralizing validation in LoadConfig is a good stepRefactoring the existing logic into
LoadConfigRawand havingLoadConfigcallconfig.validate()before returning gives you a single, consistent validation point without changing the loading strategy.ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
32-32: Tests correctly distinguish V1 (no ApiKeyFile) from V2 (with ApiKeyFile)The added
expectNullApiKeyFileflag and its use inVerifyConfig/VerifyAccounts/VerifyPropertiesalign with the adapters:
- V1 adapters surface
ApiKeyFileasnull, and tests now assert that.- V2 JSON/YAML fixtures set
Account{n}.ApiKeyFile_TEST, and the prefix-based assertion covers that.This gives good coverage of the new property without disturbing existing reflection-based checks.
Also applies to: 42-42, 50-50, 69-69, 71-75, 77-83, 87-118, 181-181
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
66-66: V1 adapter changes cleanly satisfy new interfaces without altering behaviorAdding no-op
validate()methods and exposingApiKeyFileasnullon the V1 adapters keeps legacy configs working while allowingConfigLoader.LoadConfigto callvalidate()uniformly across versions.Also applies to: 72-72, 84-84, 123-123
|
Thank you for the changes @jfly! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
67-67: V1 adapters bypass validation.The no-op
Validate()implementations in V1 adapters mean that V1 configurations skip mutual exclusivity checks and file reading logic. This allows V1 configs to have bothApiKeyandApiKeyFileset without errors.If this is intentional for backward compatibility, consider adding a comment explaining the design decision. Otherwise, consider implementing at least basic validation:
-public void Validate() { } +public void Validate() +{ + // V1 configs bypass validation for backward compatibility. + // Users should migrate to V2 format for validation support. +}Alternatively, if V1 configs should support
ApiKeyFilefunctionality, implement file reading and mutual exclusivity checks similar to V2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs(2 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(4 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ImmichFrame.WebApi/Models/ServerSettings.cs
🧰 Additional context used
🧬 Code graph analysis (3)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
Validate(8-8)Validate(27-27)Validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
Validate(27-35)Validate(74-74)Validate(94-109)ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
Validate(67-67)Validate(85-85)Validate(124-124)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
Validate(27-35)Validate(74-74)Validate(94-109)ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (3)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
Validate(67-67)Validate(85-85)Validate(124-124)ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
Validate(8-8)Validate(27-27)Validate(66-66)ImmichFrame.WebApi/Models/ServerSettings.cs (3)
Validate(27-35)Validate(74-74)Validate(94-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
52-58: LGTM!The whitespace adjustments have no functional impact.
ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
23-28: Clean separation of loading and validation.The refactoring separates configuration loading from validation, improving the design. The public
LoadConfigmethod now explicitly validates after loading, ensuring configuration errors (including mutual exclusivity violations and missing files) are caught early.ImmichFrame.Core/Interfaces/IServerSettings.cs (4)
8-8: LGTM!The addition of
Validate()toIServerSettingsestablishes a consistent validation contract across configuration types.
15-15: LGTM!The
ApiKeyFileproperty enables file-based API key storage as intended by the PR objectives.
27-27: LGTM!The addition of
Validate()toIAccountSettingsaligns with the validation contract established across all settings interfaces.
66-66: LGTM!The addition of
Validate()toIGeneralSettingscompletes the validation contract across all settings interfaces.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (4)
12-12: LGTM!Adding
ApiKeyFileto the V1 model maintains backward compatibility while supporting the new file-based configuration option.
73-73: LGTM!Exposing
ApiKeyFilethrough the adapter maintains interface compliance withIAccountSettings.
85-85: Consistent with adapter design.The no-op
Validate()inAccountSettingsV1Adapteris consistent with the overall V1 adapter pattern of bypassing validation. See the comment on line 67 for discussion of this design decision.
124-124: Consistent with adapter design.The no-op
Validate()inGeneralSettingsV1Adaptercompletes the consistent V1 adapter pattern of bypassing validation.
This reverts commit 11374c8.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
66-66: No-opValidate()in V1 adapters may deserve either delegation or an explicit commentRight now all three V1 adapter
Validate()methods are empty, while the v2ServerSettingsimplementation delegates toGeneralSettings.Validate()and eachaccount.Validate(). That means any new validation logic (like enforcing API key presence or ApiKey/ApiKeyFile semantics) will not run for legacy V1 configs.If this is intentional for backward compatibility, consider adding a brief comment like:
public void Validate() { } // V1 had no validation; keep as no-op to preserve behavior.If you eventually want consistent validation across versions, you could instead mirror the v2 pattern:
public void Validate() { GeneralSettings.Validate(); foreach (var account in Accounts) { account.Validate(); } }and then add minimal checks in the nested adapters as needed (e.g., ensure
ApiKeyis non-empty).Also applies to: 84-84, 123-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs(5 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs (4)
Validate(8-8)Validate(27-27)Validate(66-66)IServerSettings(3-7)ImmichFrame.WebApi/Models/ServerSettings.cs (5)
Validate(27-35)Validate(74-74)Validate(94-109)ServerSettings(8-26)GeneralSettings(28-63)ImmichFrame.WebApi/Helpers/Config/ConfigLoader.cs (1)
ConfigLoader(10-148)
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (1)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
ServerSettingsV1Adapter(61-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
ImmichFrame.WebApi.Tests/Resources/TestV1.json (1)
1-57: Previous mutual-exclusivity concern has been properly resolved.The prior review flagged that this test resource contained both
ApiKeyandApiKeyFile, violating the mutual-exclusivity constraint. The current state correctly contains onlyApiKey(line 3) with noApiKeyFilefield, modeling a valid V1 configuration. This separation—where V1 tests useApiKeyalone and V2 tests handleApiKeyFilesupport—is a clean design that avoids modeling invalid configurations in baseline test data.The trailing newline addition is a minor formatting improvement with no functional impact.
ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs (3)
28-68: LGTM! V1/V2 config handling correctly distinguishes ApiKeyFile expectations.The test invocations correctly pass
expectNullApiKeyFile = truefor V1 configs (which don't support file-based API keys per the V1 adapter) andfalsefor V2 configs (which do support the new option).
71-87: LGTM! Clean parameter propagation through the verification chain.The new
expectNullApiKeyFileparameter flows logically fromVerifyConfigthroughVerifyAccountstoVerifyProperties. The default value offalseinVerifyPropertiesis sensible since most configurations should have a non-null ApiKeyFile.
110-117: LGTM! ApiKeyFile validation logic correctly handles both V1 and V2 scenarios.The conditional check appropriately expects
nullfor V1 configs (which don't support file-based API keys) and the standard test pattern for V2 configs. The implementation is consistent with the existing generic property verification approach.Verify that file reading functionality and mutual exclusivity validation for
ApiKeyFilevsApiKeyare tested elsewhere, as this test focuses on config loading/parsing rather than business logic validation.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
72-72:ApiKeyFileplaceholder returningnullfor V1 is reasonableMapping
ApiKeyFiletonullhere cleanly expresses that V1 configs cannot opt into file-based API keys while still satisfying theIAccountSettingscontract. Given that the validation logic in the v2 model only acts whenApiKeyFileis non-empty, this avoids accidental file reads for legacy settings and keeps behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ImmichFrame.WebApi/Models/ServerSettings.cs (2)
27-35: Validation flow is consistent; consider clearer handling of missing accountsThe
Validatemethod correctly mirrors the adapter pattern by validating general settings and then each account. IfAccountsImplcan ever benull(e.g.,Accountsomitted in config), this will currently surface as aNullReferenceExceptionwhen iterating. If you prefer a clearer config error, you could optionally add an explicit check and throw a more descriptiveInvalidOperationExceptionwhen no accounts are configured.
94-109:ValidateAndInitializelogic is sound; watch out for non-idempotenceThe method enforces the intended rules well:
- Disallows specifying both
ApiKeyandApiKeyFile.- Reads and trims the key from
ApiKeyFilewhen provided.- Ensures that after initialization, an API key is always present (from either source), rejecting empty/whitespace-only values from the file.
One subtle edge case: after the first successful run where
ApiKeyis populated fromApiKeyFile, any subsequent call on the sameServerAccountSettingsinstance will see bothApiKeyandApiKeyFileas non-empty and throw the “Cannot specify both” exception, even though the state is actually valid. IfValidateAndInitializeis ever called more than once per instance (now or in future refactors), this ceases to be idempotent.If you want to future-proof this, consider one of:
- Tracking a “loaded from file” flag and skipping the mutual-exclusivity check when
ApiKeywas previously empty and just set fromApiKeyFile.- Or, documenting and enforcing that
ValidateAndInitializeis single-shot and only called internally viaServerSettings.Validate.Also, for consistency with the final check, you may optionally switch the first
throw new Exception(...)toInvalidOperationException.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ImmichFrame.Core/Interfaces/IServerSettings.cs(3 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(3 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ImmichFrame.Core/Interfaces/IServerSettings.cs
🧰 Additional context used
🧬 Code graph analysis (2)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
ImmichFrame.WebApi/Models/ServerSettings.cs (4)
Validate(27-35)Validate(74-74)GeneralSettings(38-75)ValidateAndInitialize(94-109)
ImmichFrame.WebApi/Models/ServerSettings.cs (2)
ImmichFrame.Core/Interfaces/IServerSettings.cs (3)
Validate(8-8)Validate(66-66)ValidateAndInitialize(27-27)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
Validate(66-73)Validate(130-130)ValidateAndInitialize(91-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (4)
66-73: Adapter-level validation wiring looks correct
ServerSettingsV1Adapter.Validatemirrors the main model’s flow by delegating toGeneralSettings.Validate()and thenValidateAndInitialize()on each account. This keeps V1 configs participating in the common validation pipeline without changing their semantics.
79-79: Explicitly returning null forApiKeyFileis appropriate for V1Hard-coding
ApiKeyFiletonullclearly documents that V1 configs don’t support file-based API keys while still satisfying the updated interface.
91-92: No-opValidateAndInitializekeeps V1 accounts compatibleAn empty
ValidateAndInitializeis a reasonable choice here: V1 accounts don’t need extra initialization, but the method being present lets shared code treat all account settings uniformly.
129-131: No-op general settings validation is fine for nowProviding a
Validatestub onGeneralSettingsV1Adaptersatisfies the interface and keeps room for future checks without impacting existing behavior.ImmichFrame.WebApi/Models/ServerSettings.cs (2)
74-74: No-opGeneralSettings.Validateis acceptableAdding a stub
ValidateonGeneralSettingskeeps the interface consistent and allows shared validation calls, without changing existing behavior.
81-81:ApiKeyFileshape and default look goodIntroducing nullable
ApiKeyFilewith anulldefault cleanly models the optional file-based credential and works well with the subsequent validation logic.
|
Sorry for the back and forth. Usually I don't like to commit to PRs directly, but I thought I'd just rename one method... I have renamed the I'll leave the PR open until you guys get a chance to comment, after I will merge it :) |
|
LGTM 👍 |
3 participants
If given,
ApiKeyFilepoints at a file that ImmichFrame will read to get the api key.ApiKeyFileis mutually exclusive withApiKey.This fixes #510
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.