|
| 1 | +--- |
| 2 | +name: code-review |
| 3 | +description: SDK-specific PR review checklist for the Contentstack .NET SDK — covers breaking changes, HTTP layer correctness, exception handling, serialization, fluent API patterns, configuration, test coverage, multi-targeting, plugin lifecycle, and internal visibility. Use when reviewing pull requests, examining code changes, or performing code quality assessments on this SDK. |
| 4 | +--- |
| 5 | + |
| 6 | +# Code Review |
| 7 | + |
| 8 | +## When to use |
| 9 | + |
| 10 | +- Reviewing a PR or diff against SDK conventions. |
| 11 | +- Self-review before opening a PR. |
| 12 | + |
| 13 | +## Severity levels |
| 14 | + |
| 15 | +- **Critical** — Must fix before merge (correctness, breaking changes, security) |
| 16 | +- **Important** — Should fix (maintainability, SDK patterns, consistency) |
| 17 | +- **Suggestion** — Consider improving (style, optimization) |
| 18 | + |
| 19 | +The sections below provide category-by-category checklists (breaking changes, HTTP, exceptions, serialization, fluent API, config, tests, multi-targeting, plugins, visibility) and a short red-flag list for quick scanning. |
| 20 | + |
| 21 | + |
| 22 | +## Code Review Checklist |
| 23 | + |
| 24 | +### Breaking Changes Checklist |
| 25 | + |
| 26 | +```markdown |
| 27 | +## Breaking Changes Review |
| 28 | +- [ ] No public method signatures removed or changed without [Obsolete] deprecation |
| 29 | +- [ ] No [JsonProperty] values changed (would break consumer deserialization silently) |
| 30 | +- [ ] No ContentstackOptions public property removed |
| 31 | +- [ ] New required options have defaults (don't break existing consumers who don't set them) |
| 32 | +- [ ] No namespace renames without backward-compatible type aliases |
| 33 | +- [ ] No IContentstackPlugin interface signature changed |
| 34 | +- [ ] Version bump planned if breaking change is intentional (Directory.Build.props) |
| 35 | +``` |
| 36 | + |
| 37 | +### HTTP Layer Checklist |
| 38 | + |
| 39 | +```markdown |
| 40 | +## HTTP Layer Review |
| 41 | +- [ ] All HTTP calls route through HttpRequestHandler.ProcessRequest |
| 42 | +- [ ] No HttpClient instantiation anywhere in the PR |
| 43 | +- [ ] New query params added to UrlQueries dict (not directly to URL string) |
| 44 | +- [ ] New field-level filters added to QueryValueJson dict |
| 45 | +- [ ] New headers added via Headers parameter to ProcessRequest |
| 46 | +- [ ] Branch header uses "branch" key, passed as separate Branch parameter |
| 47 | +- [ ] No hardcoded URLs — BaseUrl comes from Config.BaseUrl |
| 48 | +- [ ] Live preview URL resolved via Config.getBaseUrl() — not hardcoded |
| 49 | +- [ ] ProcessRequest result (string JSON) parsed, not further HTTP calls made |
| 50 | +``` |
| 51 | + |
| 52 | +### Exception Handling Checklist |
| 53 | + |
| 54 | +```markdown |
| 55 | +## Exception Handling Review |
| 56 | +- [ ] Domain-specific exception type used (QueryFilterException, AssetException, etc.) |
| 57 | +- [ ] No bare `throw new Exception(...)` or `throw new ContentstackException(...)` |
| 58 | +- [ ] All message strings sourced from ErrorMessages.cs constants |
| 59 | +- [ ] No string literals in throw statements |
| 60 | +- [ ] GetContentstackError(ex) called when catching WebException from HTTP calls |
| 61 | +- [ ] ErrorCode, StatusCode, Errors preserved when re-wrapping exceptions |
| 62 | +- [ ] New domain area has new exception class with factory methods |
| 63 | +- [ ] New error messages added to correct section in ErrorMessages.cs |
| 64 | +- [ ] FormatExceptionDetails(innerEx) used in ProcessingError factory methods |
| 65 | +``` |
| 66 | + |
| 67 | +### Serialization Checklist |
| 68 | + |
| 69 | +```markdown |
| 70 | +## Serialization Review |
| 71 | +- [ ] All public properties mapping CDA JSON fields have [JsonProperty("snake_case")] |
| 72 | +- [ ] No reliance on default Newtonsoft.Json camelCase or PascalCase matching |
| 73 | +- [ ] Custom deserialization uses [CSJsonConverter] + JsonConverter subclass |
| 74 | +- [ ] JsonConverter placed in Contentstack.Core/Internals/ (internal class) |
| 75 | +- [ ] No System.Text.Json usage |
| 76 | +- [ ] No JsonConvert.DeserializeObject with hardcoded type outside of converter |
| 77 | +- [ ] ContentstackCollection<T> used for list responses (not List<T> directly) |
| 78 | +- [ ] "entries" token used for entry collection, "assets" for asset collection |
| 79 | +``` |
| 80 | + |
| 81 | +### Fluent API Checklist |
| 82 | + |
| 83 | +```markdown |
| 84 | +## Fluent API Review |
| 85 | +- [ ] Every Query filter/operator method returns `return this;` |
| 86 | +- [ ] Null key validated at start of method → QueryFilterException.Create() |
| 87 | +- [ ] Empty string key validated → QueryFilterException.Create() |
| 88 | +- [ ] Operator value stored in QueryValueJson[key][$operator] nested dict |
| 89 | +- [ ] URL-level params stored in UrlQueries[key] |
| 90 | +- [ ] Method name follows verb+noun pattern (GreaterThan, ContainedIn, NotExists) |
| 91 | +- [ ] No mutation of QueryValueJson or UrlQueries outside of the Query class itself |
| 92 | +- [ ] And()/Or() accept Query[] (not raw dictionaries) |
| 93 | +``` |
| 94 | + |
| 95 | +### Configuration Checklist |
| 96 | + |
| 97 | +```markdown |
| 98 | +## Configuration Review |
| 99 | +- [ ] New options added to ContentstackOptions (public class), not Config (internal) |
| 100 | +- [ ] New property has XML <summary> doc comment |
| 101 | +- [ ] Default value set in ContentstackOptions() constructor or property initializer |
| 102 | +- [ ] ContentstackClient constructor reads new option and passes to Config |
| 103 | +- [ ] Config never exposed as public property |
| 104 | +- [ ] New option tested in ContentstackOptionsUnitTests.cs |
| 105 | +- [ ] ASP.NET Core binding works (IOptions<ContentstackOptions> path verified) |
| 106 | +``` |
| 107 | + |
| 108 | +### Test Coverage Checklist |
| 109 | + |
| 110 | +```markdown |
| 111 | +## Test Coverage Review |
| 112 | +- [ ] Unit test for each new public Query method (QueryValueJson assertion via reflection) |
| 113 | +- [ ] Unit test for null key input → QueryFilterException |
| 114 | +- [ ] Unit test for empty key input → QueryFilterException |
| 115 | +- [ ] Unit test for fluent return (Assert.Equal(query, result)) |
| 116 | +- [ ] Integration test file in Integration/{FeatureName}Tests/ subfolder |
| 117 | +- [ ] Integration test class extends IntegrationTestBase |
| 118 | +- [ ] Integration test constructor takes ITestOutputHelper output |
| 119 | +- [ ] CreateClient() used (not manual ContentstackClient construction) |
| 120 | +- [ ] LogArrange/LogAct/LogAssert used in correct order |
| 121 | +- [ ] TestAssert.* used (not raw Assert.*) |
| 122 | +- [ ] [Fact(DisplayName = "FeatureArea - Component Description")] present |
| 123 | +- [ ] Happy path test (valid params → expected response) |
| 124 | +- [ ] Error path test (invalid params or not found → expected exception) |
| 125 | +``` |
| 126 | + |
| 127 | +### Multi-Targeting Checklist |
| 128 | + |
| 129 | +```markdown |
| 130 | +## Multi-Targeting Review |
| 131 | +- [ ] No HttpClient (netstandard2.0 HttpClient has behavioural differences from net4x) |
| 132 | +- [ ] No System.Text.Json (not available without separate package in netstandard2.0) |
| 133 | +- [ ] No record types (C# 9, requires LangVersion setting for net47/net472) |
| 134 | +- [ ] No default interface implementations (C# 8, may affect net47) |
| 135 | +- [ ] No nullable reference types without #nullable enable guard |
| 136 | +- [ ] No top-level statements (not applicable to library projects but worth checking) |
| 137 | +- [ ] Tested compile against netstandard2.0 target (or verified via CI) |
| 138 | +``` |
| 139 | + |
| 140 | +### Plugin Lifecycle Checklist |
| 141 | + |
| 142 | +```markdown |
| 143 | +## Plugin Lifecycle Review |
| 144 | +- [ ] New feature that makes HTTP calls uses HttpRequestHandler (plugins run automatically) |
| 145 | +- [ ] No WebRequest.Create() called directly in new model classes |
| 146 | +- [ ] IContentstackPlugin interface not modified (breaking for all plugin consumers) |
| 147 | +- [ ] RequestLoggingPlugin still works with any new request/response changes |
| 148 | +- [ ] Plugin.OnRequest receives HttpWebRequest before send |
| 149 | +- [ ] Plugin.OnResponse receives response string (can mutate/inspect) |
| 150 | +``` |
| 151 | + |
| 152 | +### Internal Visibility Checklist |
| 153 | + |
| 154 | +```markdown |
| 155 | +## Internal Visibility Review |
| 156 | +- [ ] New utility/helper classes in Internals/ are marked `internal` |
| 157 | +- [ ] New model types intended for consumers are in Models/ and `public` |
| 158 | +- [ ] New configuration types are in Configuration/ and `public` |
| 159 | +- [ ] No public exposure of Config, HttpRequestHandler, or VersionUtility |
| 160 | +- [ ] InternalsVisibleTo not modified (already covers both test projects) |
| 161 | +- [ ] New internal methods accessible in unit tests without changes |
| 162 | +``` |
| 163 | + |
| 164 | +### Common Issues Found in Past PRs |
| 165 | + |
| 166 | +#### Silent Deserialization Failures |
| 167 | +`[JsonProperty]` omitted → field is always null at runtime, no exception. Verify all properties that map CDA JSON fields. |
| 168 | + |
| 169 | +#### Exception Message in Throw |
| 170 | +```csharp |
| 171 | +// Bad |
| 172 | +throw new QueryFilterException("Please provide valid params."); |
| 173 | + |
| 174 | +// Good |
| 175 | +throw QueryFilterException.Create(innerEx); |
| 176 | +// or |
| 177 | +throw new QueryFilterException(ErrorMessages.QueryFilterError); |
| 178 | +``` |
| 179 | + |
| 180 | +#### Hardcoded Environment |
| 181 | +```csharp |
| 182 | +// Bad — breaks for consumers with different environments |
| 183 | +mainJson["environment"] = "production"; |
| 184 | + |
| 185 | +// Correct — already done in Exec() |
| 186 | +mainJson["environment"] = ContentTypeInstance.StackInstance.Config.Environment; |
| 187 | +``` |
| 188 | + |
| 189 | +#### Returning void from Query Method |
| 190 | +```csharp |
| 191 | +// Bad — breaks fluent chaining |
| 192 | +public void SetMyParam(string value) { UrlQueries["my_param"] = value; } |
| 193 | + |
| 194 | +// Good |
| 195 | +public Query SetMyParam(string value) { UrlQueries["my_param"] = value; return this; } |
| 196 | +``` |
| 197 | + |
| 198 | +#### Dictionary Not Initialized for QueryValueJson Entry |
| 199 | +```csharp |
| 200 | +// Bad — throws KeyNotFoundException or InvalidCastException |
| 201 | +((Dictionary<string, object>)QueryValueJson[key])["$op"] = value; |
| 202 | + |
| 203 | +// Good — guard with ContainsKey |
| 204 | +if (!QueryValueJson.ContainsKey(key)) |
| 205 | + QueryValueJson[key] = new Dictionary<string, object>(); |
| 206 | +((Dictionary<string, object>)QueryValueJson[key])["$op"] = value; |
| 207 | +``` |
| 208 | + |
| 209 | +## SDK-specific red flags |
| 210 | + |
| 211 | +Quick scan for anti-patterns in PRs: |
| 212 | + |
| 213 | +``` |
| 214 | +❌ new HttpClient() — use HttpRequestHandler |
| 215 | +❌ throw new Exception("message") — use typed ContentstackException subclass |
| 216 | +❌ "hardcoded_field_name" — use [JsonProperty] or ErrorMessages constant |
| 217 | +❌ public Config GetConfig() — Config is internal by design |
| 218 | +❌ return void — Query methods return Query (fluent) |
| 219 | +❌ [JsonProperty] omitted — CDA uses snake_case; PascalCase won't deserialize |
| 220 | +❌ <Version> in .csproj — use Directory.Build.props |
| 221 | +``` |
| 222 | + |
| 223 | +For full category-by-category review, see **Code review checklist** above. |
0 commit comments