Fix: Make BIC optional for Girocode Version 2 #622
Conversation
- Modified IsValidBic to accept a 'required' parameter - BIC is now required for Version 1 (as per spec) - BIC is optional for Version 2 (as per spec) - Fixed NullReferenceException when BIC is null - Added comprehensive tests for all scenarios: - Version 2 with null BIC (passes) - Version 2 with empty BIC (passes) - Version 2 with valid BIC (passes) - Version 2 with invalid BIC (throws exception) - Version 1 with null BIC (throws exception) - Version 1 with empty BIC (throws exception) Fixes issue where Girocode constructor threw NullReferenceException when BIC was null with Version 2, which should be allowed per the Girocode specification.
|
Warning Rate limit exceeded@Shane32 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a BIC-validation overload that accepts a required flag, integrates it into Girocode (Version1 requires BIC, Version2 allows missing BIC), makes BIC normalization null‑safe, and adds unit tests for both versions and scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Girocode
participant Validator as IsValidBic
Caller->>Girocode: construct(..., bic, version)
Girocode->>Validator: IsValidBic(bic, required = version==V1)
alt required (Version1)
Validator-->>Girocode: false (invalid or missing)
Girocode-->>Caller: throw GirocodeException("The BIC is invalid.")
Validator-->>Girocode: true (valid)
Girocode-->>Caller: payload includes BIC
else optional (Version2)
alt bic null/empty
Validator-->>Girocode: true
Girocode-->>Caller: payload omits BIC
else bic present
Validator->>Validator: validate format (existing logic)
Validator-->>Girocode: true/false
alt valid
Girocode-->>Caller: payload includes BIC
else invalid
Girocode-->>Caller: throw GirocodeException("The BIC is invalid.")
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
QRCoder/PayloadGenerator.cs(1 hunks)QRCoder/PayloadGenerator/Girocode.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoder/PayloadGenerator.cs
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoder/PayloadGenerator/Girocode.cs (1)
QRCoder/PayloadGenerator.cs (2)
IsValidBic(65-66)IsValidBic(74-79)
QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs (2)
QRCoder/PayloadGenerator.cs (1)
PayloadGenerator(10-146)QRCoder/PayloadGenerator/Girocode.cs (8)
PayloadGenerator(3-207)Girocode(8-206)Girocode(39-69)ToString(75-95)GirocodeException(178-205)GirocodeException(183-185)GirocodeException(191-194)GirocodeException(201-204)
🪛 GitHub Actions: Format (Pull Request)
QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs
[error] 335-335: CS8625: Cannot convert null literal to non-nullable reference type.
🪛 GitHub Check: format
QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs
[failure] 425-425:
Cannot convert null literal to non-nullable reference type.
[failure] 335-335:
Cannot convert null literal to non-nullable reference type.
[failure] 425-425:
Cannot convert null literal to non-nullable reference type.
[failure] 335-335:
Cannot convert null literal to non-nullable reference type.
[failure] 425-425:
Cannot convert null literal to non-nullable reference type.
[failure] 335-335:
Cannot convert null literal to non-nullable reference type.
[failure] 425-425:
Cannot convert null literal to non-nullable reference type.
[failure] 335-335:
Cannot convert null literal to non-nullable reference type.
[failure] 425-425:
Cannot convert null literal to non-nullable reference type.
[failure] 335-335:
Cannot convert null literal to non-nullable reference type.
⏰ 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). (2)
- GitHub Check: additional-tests
- GitHub Check: build
🔇 Additional comments (1)
QRCoderTests/PayloadGeneratorTests/GirocodeTests.cs (1)
325-457: Excellent test coverage for BIC handling across versions.The test suite comprehensively covers all scenarios:
- Version2: null, empty, valid, and invalid BIC cases
- Version1: null and empty BIC validation
Test assertions correctly verify both successful payload generation and appropriate exception handling. Once the
bicparameter signature is updated to nullable in the production code, the compiler warnings will be resolved.
2 participants
Roo: I've successfully fixed the GitHub issue and completed all requested tasks:
Summary of Changes
Fixed Issue
The Girocode constructor was throwing a
NullReferenceExceptionwhen BIC was null with Version 2, which should be allowed per the Girocode specification.Changes Made
Modified
IsValidBic()in PayloadGenerator.cs:requiredparametertruefor null/empty BIC when not requiredfalsefor null/empty BIC when requiredUpdated
Girocodeconstructor in Girocode.cs:?.) and null-coalescing operator (??) for clean BIC handlingAdded comprehensive tests in GirocodeTests.cs:
Test Results
All 23 Girocode tests passed successfully across all target frameworks (net462, netcoreapp2.1, netcoreapp3.1, net5.0, net5.0-windows, net6.0, net6.0-windows).
Branch and Commit
fix/girocode-version2-bic-optionalYou can create a pull request at: https://github.com/Shane32/QRCoder/pull/new/fix/girocode-version2-bic-optional
Summary by CodeRabbit