[CDX-372] Add support for arbitrary redirect data#93
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for accessing arbitrary redirect data through an indexer pattern. The RedirectData class now allows callers to access custom metadata fields beyond the predefined Url, RuleId, and MatchId properties.
Changes:
- Added indexer support to
RedirectDataclass to access arbitrary metadata keys - Introduced JSON extension data handling via
JsonExtensionDataattribute - Added unit test validating indexer functionality with various data types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/constructor.io/models/Search/RedirectData.cs | Added dictionary-backed indexer with JsonExtensionData attribute to store arbitrary redirect metadata |
| src/Constructorio_NET.Tests/client/modules/SearchTests.cs | Added test case verifying indexer access to custom keys in redirect data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public int MatchId { get; set; } | ||
|
|
||
| [JsonExtensionData] | ||
| private Dictionary<string, object> _additionalData; |
There was a problem hiding this comment.
The private field _additionalData is not accessible to consumers who might need to enumerate all additional data keys. Consider adding a public method or property to expose the keys or entries, such as IEnumerable<string> GetAdditionalDataKeys() or a read-only view of the dictionary.
Alexey-Pavlov
left a comment
Alexey-Pavlov
left a comment
There was a problem hiding this comment.
LGTM! Thanks so much! I just left a small nitpick
| public object this[string key] | ||
| { | ||
| get => _additionalData?.TryGetValue(key, out var value) == true ? value : null; | ||
| set |
There was a problem hiding this comment.
Do we really need a setter here? RedirectData comes from the API response, users read it, they don't construct it themselves AFAIU 🤔
There was a problem hiding this comment.
That's fair. I thought about this for a bit as well. I thought it might be helpful for testing in the future and I wanted to keep it consistent with other attributes of this call. But I agree, it's not necessary 🤔
No description provided.