native adapters - #123
Conversation
📝 WalkthroughWalkthroughThis PR introduces native adapter infrastructure to the Bitween system. A new reflection-based discovery service identifies local adapters implementing handler, validator, and receiver interfaces. Existing resource handlers and services are updated to route requests prefixed with "native." to native adapters, or fall back to serverless adapters. Includes an HTTP handler native adapter implementation with authentication support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Resource Handler<br/>(GetProperties/Search)
participant Discovery as NativeAdapterDiscoveryService
participant NativeAdapter as Native Adapter<br/>(Cached)
participant Serverless as Serverless Service
Client->>Handler: Request with adapter key
alt Native Adapter (key starts with "native.")
Handler->>Discovery: GetNativeAdapters(prefix)<br/>or GetNativeAdapterProperties(key)
Discovery->>Discovery: Return from cache
Discovery-->>Handler: Adapter metadata/properties
Handler-->>Client: Native adapter results
else Serverless Adapter
Handler->>Serverless: Start service & gather properties
Serverless->>Serverless: Initialize and collect startup values
Serverless-->>Handler: Property dictionary
Handler-->>Client: Serverless adapter results
end
sequenceDiagram
participant Client
participant XchangeService
participant Discovery as NativeAdapterDiscoveryService
participant NativeAdapter as Native Adapter<br/>(HttpHandler etc.)
participant Serverless as IServerlessService
Client->>XchangeService: Invoke mapper/handler/validator
alt Native Adapter Path (id starts with "native.")
XchangeService->>Discovery: GetNativeAdapterInfo(adapterId)
Discovery-->>XchangeService: Adapter metadata + type
XchangeService->>XchangeService: InstantiateNativeAdapter<T>()<br/>(Map properties dict to input model)
XchangeService->>NativeAdapter: Instantiate adapter with mapped input
XchangeService->>NativeAdapter: Invoke method (Handle/Validate/etc.)
NativeAdapter-->>XchangeService: Result
XchangeService-->>Client: Return result
else Serverless Path
XchangeService->>Serverless: Start service
Serverless->>Serverless: Initialize and process
Serverless-->>XchangeService: Response
XchangeService-->>Client: Return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 14
🤖 Fix all issues with AI agents
In `@SW.Bitween.Api/Resources/Subscriptions/Update.cs`:
- Around line 111-112: Replace calls to serviceProvider.GetService<T>() with
serviceProvider.GetRequiredService<T>() for the referenced services so the app
fails fast with a clear exception instead of risking a NullReferenceException;
specifically update the usages of NativeAdapterDiscoveryService (used before
calling GetNativeAdapterProperties(mapperId)) and the IServerlessService lookups
(the calls around where you later call methods on the returned instance) to use
GetRequiredService on the serviceProvider variable in Update.cs so the service
resolution throws immediately if the registration is missing.
In `@SW.Bitween.Api/Services/NativeAdapterDiscoveryService.cs`:
- Around line 92-97: GetNativeAdapterInfo can return null because it uses
FirstOrDefault, so update the method signature to return NativeAdapterInfo?
(nullable) and keep the existing FirstOrDefault usage; ensure callers (e.g.,
ReceivingService and any other callers) continue to null-check the result or are
updated to handle the nullable return appropriately. Locate the method
GetNativeAdapterInfo and change its return type to NativeAdapterInfo? so the
nullability is explicit.
- Around line 48-53: The bare catch in NativeAdapterDiscoveryService silently
swallows all exceptions during assembly scanning; change it to catch (Exception
ex) and log a warning that includes the assembly name and exception details
using an injected ILogger<NativeAdapterDiscoveryService> (replace any
Debug.WriteLine calls with logger.LogWarning or logger.LogError and use
structured logging like "Failed to load/scan assembly {AssemblyName}:
{Exception}"). Ensure ILogger<NativeAdapterDiscoveryService> is added to the
service via constructor injection and used inside the catch block to record the
assembly name and ex.
In `@SW.Bitween.Api/Services/ReceivingService.cs`:
- Around line 81-104: Replace nullable service resolution with required
resolution: in ReceivingService where you call
serviceProvider.GetService<XchangeService>() (used after
InstantiateNativeReceiver and later in the serverless path), switch to
serviceProvider.GetRequiredService<XchangeService>() so XchangeService is
guaranteed and a clear error is thrown if not registered; update both
occurrences that currently use GetService<XchangeService>() before calling
SubmitSubscriptionXchange on the xchangeService instance.
In `@SW.Bitween.Api/Services/XchangeService.cs`:
- Around line 207-256: The InstantiateNativeAdapter method currently picks any
constructor with parameters and uses Activator.CreateInstance(inputType) which
fails if the input model lacks a public parameterless ctor; change selection to
prefer a single-parameter constructor (replace FirstOrDefault(c =>
c.GetParameters().Length > 0) with FirstOrDefault(c => c.GetParameters().Length
== 1) or otherwise order by parameter count explicitly) and then build the input
instance by either: (a) if the inputType has a public parameterless constructor,
continue mapping properties as before; or (b) if it does not, map the adapter
constructor's parameters from the properties dictionary and invoke that adapter
constructor directly (use constructor.Invoke with an object[] of converted
parameter values) or throw a clear BitweenException if required parameter values
are missing/convertible; ensure you reference InstantiateNativeAdapter,
adapterInfo.Type, constructor, inputType, inputInstance and replace the
Activator.CreateInstance(adapterInfo.Type, inputInstance) call with the
appropriate constructor.Invoke call when using a constructor that takes the
mapped parameter(s).
In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs`:
- Around line 12-25: The HttpMethodFromString method currently maps unknown
verbs to POST and misses PATCH; update the switch in HttpMethodFromString to add
a "patch" case returning HttpMethod.Patch (or new HttpMethod("PATCH")) and
replace the silent default with throwing an informative exception (e.g.,
ArgumentException) for truly unrecognized method strings so consumers aren't
accidentally using POST when they meant PATCH.
- Around line 66-72: The OAuth2 branch uses null-forgiving on _options.ClientId,
_options.ClientSecret and _options.LoginUrl which can cause NREs; update the
OAuth2 branch (the else if (_options.AuthType == "OAuth2") block and the Login
branch that uses LoginUrl) to validate those nullable properties up front (e.g.,
check _options.ClientId, _options.ClientSecret, and _options.LoginUrl for null
or empty) and handle missing values by throwing a clear
ArgumentException/InvalidOperationException or returning a handled error/log via
processLogger before constructing the HttpRequestMessage; ensure validation is
centralized (add a private ValidateAuthOptions or similar) and reference it from
the OAuth2 branch and the Login branch to avoid repeated null-forgiving
operators.
- Around line 75-79: The OAuth2 token response handling does not verify success
and can set a "Bearer null" header; update the code around oauthResponse /
oathRequest / OAuth2Response to first check oauthResponse.IsSuccessStatusCode
(and/or validate resDeserialized != null and resDeserialized.access_token is not
null/empty) before assigning client.DefaultRequestHeaders.Authorization, and if
the response failed, throw or return a clear error that includes the
status/reason/content (e.g., oauthResponse.StatusCode,
oauthResponse.ReasonPhrase or the response body) so callers don't proceed with
an invalid token.
- Around line 56-64: The code calls loginResponse.EnsureSuccessStatusCode() and
then redundantly checks loginResponse.StatusCode != HttpStatusCode.OK which
incorrectly rejects other valid 2xx responses; remove the second conditional and
rely on EnsureSuccessStatusCode() to validate success, then read the response
body and set the Authorization header from rsDeserialized?.Jwt (ensure
LoginResponse deserialization and Jwt null-handling remain intact) — look for
symbols loginResponse, EnsureSuccessStatusCode(), rsDeserialized, Jwt, and
client.DefaultRequestHeaders.Authorization in HttpHandler.cs to apply the
change.
- Line 36: The HttpHandler currently creates a new HttpClient with "HttpClient
client = new HttpClient();" which risks socket exhaustion; fix it by making
HttpClient a long-lived dependency: add an IHttpClientFactory (or a shared
HttpClient) injected via the HttpHandler constructor and use
factory.CreateClient(...) inside the request method, or at minimum wrap the
existing instantiation in a using/block to ensure disposal; update the
constructor signature and any call sites to accept IHttpClientFactory (or store
a static/shared HttpClient) and remove the per-call "new HttpClient()" in the
HttpHandler class.
- Around line 129-136: The header parsing currently uses h.Split(':') and
accesses strArray[1], which throws on malformed entries and truncates values
containing ':'; update the logic in HttpHandler.cs (the headers1/headers
creation) to call h.Split(new[] { ':' }, 2) (or Split(':', 2)) so it only splits
into two parts, then check parts.Length == 2 before creating the KeyValuePair,
Trim() both key and value, and skip or log any malformed entries instead of
indexing out of range; ensure behavior when _options.Headers is null remains
unchanged.
In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs`:
- Around line 36-49: The WriteObject method currently emits an empty object for
non-IDictionary values; update WriteObject (in DictionaryConverter.cs) so that
after casting to IDictionary<string, object> it falls back for non-dictionary
types by delegating to JsonSerializer.Serialize(writer, value) (or throw a
descriptive JsonSerializationException if you prefer fail-fast) instead of
silently writing {}. Ensure the chosen approach integrates with the existing
WriteValue/WriteObject flow (add a JsonSerializer parameter or call
JsonSerializer.CreateDefault() as appropriate) and include a clear error message
if you opt to throw.
- Around line 99-120: ReadArray and ReadObject currently drop entries when
ReadValue returns null (e.g., JSON null), which loses fidelity; update both
methods (ReadArray and ReadObject) so that when ReadValue returns null you
explicitly add/store a null value (e.g., add null to the IList<object> in
ReadArray and set the dictionary value to null in ReadObject) instead of
skipping, or if the omission was intentional add a clear comment explaining the
behavior; ensure you reference the existing ReadValue call sites in ReadArray
and the property-assignment block in ReadObject and adjust them to preserve
nulls.
- Around line 13-34: The WriteValue method currently materializes a JToken via
JToken.FromObject(value) which is wasteful and may re-enter the converter;
replace that logic with direct runtime type checks: if value is
IDictionary<string, object> (or IDictionary) call WriteObject(writer, value),
else if value is IEnumerable and not a string (e.g. value is IEnumerable
enumerable && !(value is string)) call WriteArray(writer, value), otherwise call
writer.WriteValue(value); update the branching in WriteValue (refer to
WriteValue, WriteObject, WriteArray) to use these checks and remove the
JToken.FromObject usage to prevent recursion and improve performance.
🧹 Nitpick comments (16)
SW.Bitween.NativeAdapters/HttpHandler/HttpHandlerModels.cs (2)
1-7: Namespace doesn't match folder structure.The file is under
HttpHandler/but the namespace isSW.Bitween.NativeAdaptersrather thanSW.Bitween.NativeAdapters.HttpHandler. This could cause confusion and conflicts if other model files in different subfolders also use the root namespace.
15-18: Use PascalCase for the property name with a[JsonProperty]attribute.
access_tokenbreaks C# naming conventions. Since Newtonsoft.Json is already a dependency, use[JsonProperty]to handle the JSON mapping while keeping the property idiomatic.♻️ Proposed fix
+using Newtonsoft.Json; + public class OAuth2Response { - public string? access_token { get; set; } + [JsonProperty("access_token")] + public string? AccessToken { get; set; } }SW.Bitween.NativeAdapters/HttpHandler/HttpHandlerInput.cs (2)
4-4: Namespace doesn't reflect theHttpHandlersubfolder.The file lives under
HttpHandler/but the namespace isSW.Bitween.NativeAdapters. If this is intentional (keeping a flat namespace for the project), that's fine — but it may cause naming collisions as more adapters are added. ConsiderSW.Bitween.NativeAdapters.HttpHandlerto match the folder structure.
8-28: Add [JsonIgnore] to sensitive credential properties as a defensive best practice.The properties
ApiKey,LoginPassword, andClientSecretcontain secrets that should be protected from accidental exposure through serialization or logging. While the current code extracts these values directly from_optionsfor use in auth flows rather than serializing the entire object, adding[JsonIgnore]prevents future risks if this class is ever inadvertently serialized for debugging, logging, or reflection-based inspection.Newtonsoft.Json is already a project dependency, so implementation is straightforward.
Additionally, consider grouping authentication-related properties (
AuthType,LoginUrl,LoginUsername,LoginPassword) together for improved readability.Example: protect secrets with JsonIgnore
+using Newtonsoft.Json; + public class HttpHandlerInput { public string? AuthType { get; set; } + public string? LoginUrl { get; set; } + public string? LoginUsername { get; set; } + [JsonIgnore] + public string? LoginPassword { get; set; } + + [JsonIgnore] public string? ApiKey { get; set; } - public string? LoginUrl { get; set; } [DefaultValue("post")] public string Verb { get; set; } = "post"; - public string? LoginUsername { get; set; } - public string? LoginPassword { get; set; } - [Required] public string Url { get; set; } = string.Empty; [DefaultValue("application/json")] public string ContentType { get; set; } = "application/json"; public string? Headers { get; set; } public string? CorrelationId { get; set; } public string? ClientId { get; set; } + [JsonIgnore] public string? ClientSecret { get; set; } public string? DefaultRequest { get; set; } }SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs (1)
123-128: Request body is attached even for GET and DELETE requests.
Contentis always set on theHttpRequestMessage, regardless of the HTTP verb. While technically allowed by HTTP, many servers and proxies reject or ignore bodies on GET/DELETE. Consider only settingContentfor POST/PUT/PATCH.SW.Bitween.Web/SW.Bitween.Web.csproj (1)
37-37: Potentially redundant project reference.
SW.Bitween.Webalready referencesSW.Bitween.Api, which itself referencesSW.Bitween.NativeAdapters. This direct reference is only needed ifSW.Bitween.Webdirectly uses types fromNativeAdapters(e.g., inStartup.csDI registration). If so, this is fine — just flagging for awareness.SW.Bitween.Api/Services/NativeAdapterDiscoveryService.cs (3)
63-64: Remove commented-out code.Lines 63-64 contain commented-out name transformation logic. If this is no longer needed, remove it to keep the codebase clean. If it's planned for future use, track it in an issue instead.
157-162: Redundant checks inIsNullableType.
Nullable.GetUnderlyingType(type) != nullalready covers thetype.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>)check — they're equivalent. One of the two can be removed.♻️ Simplified version
private bool IsNullableType(Type type) { - return !type.IsValueType || - Nullable.GetUnderlyingType(type) != null || - (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>)); + return !type.IsValueType || Nullable.GetUnderlyingType(type) != null; }
20-26: Inconsistent indentation inDiscoverNativeAdapters.The
foreachon line 26 appears to be at a different indentation level than the enclosing method body. This looks like a merge/paste artifact. While it compiles, it hurts readability.SW.Bitween.Api/Resources/Subscriptions/Update.cs (1)
103-127: Significant code duplication across three validation blocks.The mapper, handler, and receiver validation blocks are nearly identical — differing only in the adapter ID source and the property collection. Consider extracting a shared helper method to reduce duplication and maintenance burden.
♻️ Example helper extraction
private static async Task ValidateAdapterProperties( IServiceProvider serviceProvider, string adapterId, ICollection<KeyAndValue> properties, ValidationContext<SubscriptionUpdate> context, string errorPrefix = "Missing") { var mustProps = Enumerable.Empty<string>(); if (adapterId.StartsWith("native.", StringComparison.OrdinalIgnoreCase)) { var nativeAdapterDiscovery = serviceProvider.GetRequiredService<NativeAdapterDiscoveryService>(); var adapterProps = nativeAdapterDiscovery.GetNativeAdapterProperties(adapterId); mustProps = adapterProps.Where(p => p.Value.EndsWith(" *")).Select(p => p.Key); } else { var serverless = serviceProvider.GetRequiredService<IServerlessService>(); await serverless.StartAsync(adapterId, null); mustProps = (await serverless.GetExpectedStartupValues()) .Where(p => p.Value.Optional == false).Select(p => p.Key); } var missing = mustProps.ToHashSet(StringComparer.OrdinalIgnoreCase) .Except(properties.Where(p => !string.IsNullOrEmpty(p.Value)).Select(p => p.Key)); if (missing.Any()) context.AddFailure($"{errorPrefix}: {string.Join(",", missing)}"); }Also applies to: 132-157, 166-190
SW.Bitween.Api/Resources/Adapters/SearchVersioned.cs (1)
56-67: The two anonymous types differ — consider using a shared DTO.
nativeAdaptersandexternalAdaptershave structurally different anonymous types (theVersionslists contain different element types).Concat<object>works at runtime but makes the return shape dependent on serializer behavior for anonymous types. If the API consumer expects a uniform schema for each entry'sVersionsarray, this could produce inconsistent JSON shapes — native entries get[]while external entries get[{Key: "..."}].This is fine for now if the consumer handles it, but a shared DTO would make the contract explicit and prevent serialization surprises.
SW.Bitween.Api/Resources/Adapters/GetProperties.cs (1)
25-36: Native adapter path looks clean; note silent empty-result on unknown adapter.If
decodedKeystarts with"native."but doesn't match any registered adapter,GetNativeAdapterPropertiessilently returns an empty dictionary. This may be intentional, but it could mask configuration errors where a user specifies a nonexistent native adapter key. Consider whether a 404 or error response would be more helpful for debugging.SW.Bitween.Api/Services/ReceivingService.cs (4)
90-90: Use.Countproperty instead of.Count()LINQ extension onList<T>.
fileListis already aList, so.Count(property) avoids the overhead of the LINQ extension call. Same applies on line 114.
131-181:InstantiateNativeReceiveris nearly identical toXchangeService.InstantiateNativeAdapter<T>— extract shared helper.The reflection-based instantiation logic (adapter lookup → constructor introspection → input model creation → property mapping → adapter creation) is duplicated almost verbatim between this method and
XchangeService.InstantiateNativeAdapter<T>. This duplication means bug fixes or enhancements (e.g., better type conversion, validation of required properties) must be applied in both places.Consider extracting this into
NativeAdapterDiscoveryService(or a dedicated factory class) so both consumers share one implementation.
162-173: Silent swallow of conversion failures may cause adapters to run with missing configuration.When
Convert.ChangeTypethrows (e.g., for enums, GUIDs, or complex types), the catch block only sets the value if the property is astring. For all other types, the property silently retains its default value. This could cause hard-to-debug runtime failures in adapters that expect configured values.At minimum, log the conversion failure. Better yet, consider using
TypeDescriptor.GetConverterwhich handles a broader range of conversions (enums, GUIDs, etc.).♻️ Suggested improvement: log + use TypeDescriptor
try { - var convertedValue = Convert.ChangeType(value, - Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType); - prop.SetValue(inputInstance, convertedValue); + var targetType = Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType; + var converter = System.ComponentModel.TypeDescriptor.GetConverter(targetType); + var convertedValue = converter.ConvertFromInvariantString(value); + prop.SetValue(inputInstance, convertedValue); } catch { - // If conversion fails, set string value directly if (prop.PropertyType == typeof(string)) prop.SetValue(inputInstance, value); + // else: log warning about failed conversion }
184-235: Consider removing commented-out dead code.Large block of commented-out code (old timer-based implementation) adds noise. If this code is no longer needed, remove it; version control preserves history.
| var nativeAdapterDiscovery = serviceProvider.GetService<NativeAdapterDiscoveryService>(); | ||
| var properties = nativeAdapterDiscovery.GetNativeAdapterProperties(mapperId); |
There was a problem hiding this comment.
Use GetRequiredService instead of GetService to fail fast.
GetService<NativeAdapterDiscoveryService>() returns null if the service isn't registered, and the very next line dereferences it without a null check. Use GetRequiredService to get a clear exception instead of a NullReferenceException. Same applies to GetService<IServerlessService>() on lines 117, 146, and 180.
🐛 Proposed fix (apply to all three blocks)
- var nativeAdapterDiscovery = serviceProvider.GetService<NativeAdapterDiscoveryService>();
+ var nativeAdapterDiscovery = serviceProvider.GetRequiredService<NativeAdapterDiscoveryService>();- var serverless = serviceProvider.GetService<IServerlessService>();
+ var serverless = serviceProvider.GetRequiredService<IServerlessService>();Also applies to: 140-141, 174-175
🤖 Prompt for AI Agents
In `@SW.Bitween.Api/Resources/Subscriptions/Update.cs` around lines 111 - 112,
Replace calls to serviceProvider.GetService<T>() with
serviceProvider.GetRequiredService<T>() for the referenced services so the app
fails fast with a clear exception instead of risking a NullReferenceException;
specifically update the usages of NativeAdapterDiscoveryService (used before
calling GetNativeAdapterProperties(mapperId)) and the IServerlessService lookups
(the calls around where you later call methods on the returned instance) to use
GetRequiredService on the serviceProvider variable in Update.cs so the service
resolution throws immediately if the registration is missing.
| } | ||
| catch | ||
| { | ||
| // Skip assemblies that can't be loaded or scanned | ||
| } | ||
| } |
There was a problem hiding this comment.
Bare catch swallows all exceptions silently.
Catching all exceptions without logging makes it very difficult to diagnose adapter discovery failures. At minimum, log a warning with the exception and assembly name.
🔧 Proposed fix
catch
+ catch (Exception ex)
{
- // Skip assemblies that can't be loaded or scanned
+ // Log warning but continue discovery for other assemblies
+ System.Diagnostics.Debug.WriteLine($"Failed to scan assembly {assembly.FullName}: {ex.Message}");
}Ideally, inject an ILogger<NativeAdapterDiscoveryService> and use structured logging instead of Debug.WriteLine.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| catch | |
| { | |
| // Skip assemblies that can't be loaded or scanned | |
| } | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| // Log warning but continue discovery for other assemblies | |
| System.Diagnostics.Debug.WriteLine($"Failed to scan assembly {assembly.FullName}: {ex.Message}"); | |
| } |
🤖 Prompt for AI Agents
In `@SW.Bitween.Api/Services/NativeAdapterDiscoveryService.cs` around lines 48 -
53, The bare catch in NativeAdapterDiscoveryService silently swallows all
exceptions during assembly scanning; change it to catch (Exception ex) and log a
warning that includes the assembly name and exception details using an injected
ILogger<NativeAdapterDiscoveryService> (replace any Debug.WriteLine calls with
logger.LogWarning or logger.LogError and use structured logging like "Failed to
load/scan assembly {AssemblyName}: {Exception}"). Ensure
ILogger<NativeAdapterDiscoveryService> is added to the service via constructor
injection and used inside the catch block to record the assembly name and ex.
| public NativeAdapterInfo GetNativeAdapterInfo(string adapterId) | ||
| { | ||
| return _adaptersCache.Values | ||
| .SelectMany(v => v) | ||
| .FirstOrDefault(a => a.Key.Equals(adapterId, StringComparison.OrdinalIgnoreCase)); | ||
| } |
There was a problem hiding this comment.
GetNativeAdapterInfo can return null but the return type is non-nullable.
FirstOrDefault returns null when no match is found, but the method signature declares NativeAdapterInfo (not NativeAdapterInfo?). Callers (e.g., ReceivingService.cs line 135) already null-check the result, but the signature should reflect nullability to prevent misuse.
Proposed fix
- public NativeAdapterInfo GetNativeAdapterInfo(string adapterId)
+ public NativeAdapterInfo? GetNativeAdapterInfo(string adapterId)🤖 Prompt for AI Agents
In `@SW.Bitween.Api/Services/NativeAdapterDiscoveryService.cs` around lines 92 -
97, GetNativeAdapterInfo can return null because it uses FirstOrDefault, so
update the method signature to return NativeAdapterInfo? (nullable) and keep the
existing FirstOrDefault usage; ensure callers (e.g., ReceivingService and any
other callers) continue to null-check the result or are updated to handle the
nullable return appropriately. Locate the method GetNativeAdapterInfo and change
its return type to NativeAdapterInfo? so the nullability is explicit.
| // Check if it's a native adapter | ||
| if (serverlessId.StartsWith("native.", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var nativeAdapterDiscovery = serviceProvider.GetRequiredService<NativeAdapterDiscoveryService>(); | ||
| var receiver = InstantiateNativeReceiver(nativeAdapterDiscovery, serverlessId, startupParameters); | ||
|
|
||
| await receiver.Initialize(); | ||
| var fileList = (await receiver.ListFiles()).ToList(); | ||
|
|
||
| logger.LogInformation($"Subscription:'{subId}' found {fileList.Count()} items for retrieval."); | ||
|
|
||
| foreach (var file in fileList) | ||
| { | ||
| var xchangeFile = await receiver.GetFile(file); | ||
|
|
||
| logger.LogInformation($"Subscription:'{subId}' found {fileList.Count()} items for retrieval."); | ||
| logger.LogInformation($"Submitting received file for subscriber: '{subId}'."); | ||
|
|
||
| foreach (var file in fileList) | ||
| var xchangeService = serviceProvider.GetService<XchangeService>(); | ||
| await xchangeService.SubmitSubscriptionXchange(subId, xchangeFile); | ||
| await receiver.DeleteFile(file); | ||
| } | ||
|
|
||
| await receiver.Finalize(); | ||
| } |
There was a problem hiding this comment.
GetService<XchangeService>() can return null — use GetRequiredService.
On line 98, serviceProvider.GetService<XchangeService>() returns null if XchangeService isn't registered in DI, causing a NullReferenceException on line 99. The same pattern appears on line 122 for the serverless path. The rest of the codebase (lines 84, 108) correctly uses GetRequiredService.
🐛 Proposed fix
- var xchangeService = serviceProvider.GetService<XchangeService>();
+ var xchangeService = serviceProvider.GetRequiredService<XchangeService>();Apply at both line 98 and line 122.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if it's a native adapter | |
| if (serverlessId.StartsWith("native.", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var nativeAdapterDiscovery = serviceProvider.GetRequiredService<NativeAdapterDiscoveryService>(); | |
| var receiver = InstantiateNativeReceiver(nativeAdapterDiscovery, serverlessId, startupParameters); | |
| await receiver.Initialize(); | |
| var fileList = (await receiver.ListFiles()).ToList(); | |
| logger.LogInformation($"Subscription:'{subId}' found {fileList.Count()} items for retrieval."); | |
| foreach (var file in fileList) | |
| { | |
| var xchangeFile = await receiver.GetFile(file); | |
| logger.LogInformation($"Subscription:'{subId}' found {fileList.Count()} items for retrieval."); | |
| logger.LogInformation($"Submitting received file for subscriber: '{subId}'."); | |
| foreach (var file in fileList) | |
| var xchangeService = serviceProvider.GetService<XchangeService>(); | |
| await xchangeService.SubmitSubscriptionXchange(subId, xchangeFile); | |
| await receiver.DeleteFile(file); | |
| } | |
| await receiver.Finalize(); | |
| } | |
| // Check if it's a native adapter | |
| if (serverlessId.StartsWith("native.", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var nativeAdapterDiscovery = serviceProvider.GetRequiredService<NativeAdapterDiscoveryService>(); | |
| var receiver = InstantiateNativeReceiver(nativeAdapterDiscovery, serverlessId, startupParameters); | |
| await receiver.Initialize(); | |
| var fileList = (await receiver.ListFiles()).ToList(); | |
| logger.LogInformation($"Subscription:'{subId}' found {fileList.Count()} items for retrieval."); | |
| foreach (var file in fileList) | |
| { | |
| var xchangeFile = await receiver.GetFile(file); | |
| logger.LogInformation($"Submitting received file for subscriber: '{subId}'."); | |
| var xchangeService = serviceProvider.GetRequiredService<XchangeService>(); | |
| await xchangeService.SubmitSubscriptionXchange(subId, xchangeFile); | |
| await receiver.DeleteFile(file); | |
| } | |
| await receiver.Finalize(); | |
| } |
🤖 Prompt for AI Agents
In `@SW.Bitween.Api/Services/ReceivingService.cs` around lines 81 - 104, Replace
nullable service resolution with required resolution: in ReceivingService where
you call serviceProvider.GetService<XchangeService>() (used after
InstantiateNativeReceiver and later in the serverless path), switch to
serviceProvider.GetRequiredService<XchangeService>() so XchangeService is
guaranteed and a clear error is thrown if not registered; update both
occurrences that currently use GetService<XchangeService>() before calling
SubmitSubscriptionXchange on the xchangeService instance.
| private T InstantiateNativeAdapter<T>(string adapterId, IDictionary<string, string> properties) | ||
| { | ||
| var adapterInfo = _nativeAdapterDiscovery.GetNativeAdapterInfo(adapterId); | ||
| if (adapterInfo == null) | ||
| throw new BitweenException($"Native adapter not found: {adapterId}"); | ||
|
|
||
| // Get the constructor that takes a parameter | ||
| var constructor = adapterInfo.Type.GetConstructors() | ||
| .FirstOrDefault(c => c.GetParameters().Length > 0); | ||
|
|
||
| if (constructor == null) | ||
| throw new BitweenException($"Native adapter {adapterId} must have a constructor that accepts an input model"); | ||
|
|
||
| // Get the input parameter type | ||
| var inputParameter = constructor.GetParameters().First(); | ||
| var inputType = inputParameter.ParameterType; | ||
|
|
||
| // Create an instance of the input model by mapping properties | ||
| var inputInstance = Activator.CreateInstance(inputType); | ||
|
|
||
| // Map dictionary properties to the input model | ||
| foreach (var prop in inputType.GetProperties()) | ||
| { | ||
| // Case-insensitive property lookup | ||
| var propEntry = properties.FirstOrDefault(p => | ||
| string.Equals(p.Key, prop.Name, StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| if (!string.IsNullOrEmpty(propEntry.Key)) | ||
| { | ||
| var value = propEntry.Value; | ||
| try | ||
| { | ||
| var convertedValue = Convert.ChangeType(value, | ||
| Nullable.GetUnderlyingType(prop.PropertyType) ?? prop.PropertyType); | ||
| prop.SetValue(inputInstance, convertedValue); | ||
| } | ||
| catch | ||
| { | ||
| // If conversion fails, set string value directly | ||
| if (prop.PropertyType == typeof(string)) | ||
| prop.SetValue(inputInstance, value); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Instantiate the adapter with the input model | ||
| var adapter = Activator.CreateInstance(adapterInfo.Type, inputInstance); | ||
|
|
||
| return (T)adapter; | ||
| } |
There was a problem hiding this comment.
Constructor selection is fragile — ambiguous when multiple constructors exist.
FirstOrDefault(c => c.GetParameters().Length > 0) picks an arbitrary constructor if the adapter type has more than one constructor with parameters. This could silently select the wrong constructor, leading to a runtime failure at Activator.CreateInstance (line 253) or incorrect input mapping.
Consider selecting the constructor with exactly one parameter (the input model), or ordering by parameter count to pick the most specific one.
Additionally, Activator.CreateInstance(inputType) on line 225 requires a public parameterless constructor on the input model type. If the input model is a record with primary constructor parameters or lacks a default constructor, this will throw MissingMethodException.
🛡️ Suggested fix: tighten constructor selection
// Get the constructor that takes a parameter
var constructor = adapterInfo.Type.GetConstructors()
- .FirstOrDefault(c => c.GetParameters().Length > 0);
+ .Where(c => c.GetParameters().Length == 1)
+ .FirstOrDefault();🤖 Prompt for AI Agents
In `@SW.Bitween.Api/Services/XchangeService.cs` around lines 207 - 256, The
InstantiateNativeAdapter method currently picks any constructor with parameters
and uses Activator.CreateInstance(inputType) which fails if the input model
lacks a public parameterless ctor; change selection to prefer a single-parameter
constructor (replace FirstOrDefault(c => c.GetParameters().Length > 0) with
FirstOrDefault(c => c.GetParameters().Length == 1) or otherwise order by
parameter count explicitly) and then build the input instance by either: (a) if
the inputType has a public parameterless constructor, continue mapping
properties as before; or (b) if it does not, map the adapter constructor's
parameters from the properties dictionary and invoke that adapter constructor
directly (use constructor.Invoke with an object[] of converted parameter values)
or throw a clear BitweenException if required parameter values are
missing/convertible; ensure you reference InstantiateNativeAdapter,
adapterInfo.Type, constructor, inputType, inputInstance and replace the
Activator.CreateInstance(adapterInfo.Type, inputInstance) call with the
appropriate constructor.Invoke call when using a constructor that takes the
mapped parameter(s).
| var oauthResponse = await client.SendAsync(oathRequest); | ||
| var res = await oauthResponse.Content.ReadAsStringAsync(); | ||
| var resDeserialized = JsonConvert.DeserializeObject<OAuth2Response>(res); | ||
| client.DefaultRequestHeaders.Authorization = | ||
| new AuthenticationHeaderValue("Bearer", resDeserialized?.access_token); |
There was a problem hiding this comment.
OAuth2 token response is not checked for success.
Unlike the Login branch, the OAuth2 branch never checks whether the token request succeeded. A 4xx/5xx response will silently deserialize to a null access_token, resulting in a Bearer null header on subsequent requests.
Proposed fix
var oauthResponse = await client.SendAsync(oathRequest);
+ oauthResponse.EnsureSuccessStatusCode();
var res = await oauthResponse.Content.ReadAsStringAsync();🤖 Prompt for AI Agents
In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs` around lines 75 - 79,
The OAuth2 token response handling does not verify success and can set a "Bearer
null" header; update the code around oauthResponse / oathRequest /
OAuth2Response to first check oauthResponse.IsSuccessStatusCode (and/or validate
resDeserialized != null and resDeserialized.access_token is not null/empty)
before assigning client.DefaultRequestHeaders.Authorization, and if the response
failed, throw or return a clear error that includes the status/reason/content
(e.g., oauthResponse.StatusCode, oauthResponse.ReasonPhrase or the response
body) so callers don't proceed with an invalid token.
| string? headers1 = _options.Headers; | ||
| IEnumerable<KeyValuePair<string, string>>? headers = headers1 != null | ||
| ? (headers1.Split(',')).Select((Func<string, KeyValuePair<string, string>>)(h => | ||
| { | ||
| string[] strArray = h.Split(':'); | ||
| return new KeyValuePair<string, string>(strArray[0], strArray[1]); | ||
| })) | ||
| : null; |
There was a problem hiding this comment.
Header parsing crashes on malformed input and truncates values containing :.
h.Split(':') followed by strArray[1] throws IndexOutOfRangeException if a header entry has no :. It also drops everything after the first : in the value (e.g., Authorization:Bearer xyz → value is only Bearer xyz — actually that works since Split only gives two parts... no, Split(':') gives all parts). For a value like X-Custom:http://example.com, strArray[1] would be http — the rest is lost.
Use Split(':', 2) and add a bounds check:
Proposed fix
- string[] strArray = h.Split(':');
- return new KeyValuePair<string, string>(strArray[0], strArray[1]);
+ string[] strArray = h.Trim().Split(':', 2);
+ if (strArray.Length < 2)
+ throw new ArgumentException($"Invalid header format: '{h}'. Expected 'key:value'.");
+ return new KeyValuePair<string, string>(strArray[0].Trim(), strArray[1].Trim());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string? headers1 = _options.Headers; | |
| IEnumerable<KeyValuePair<string, string>>? headers = headers1 != null | |
| ? (headers1.Split(',')).Select((Func<string, KeyValuePair<string, string>>)(h => | |
| { | |
| string[] strArray = h.Split(':'); | |
| return new KeyValuePair<string, string>(strArray[0], strArray[1]); | |
| })) | |
| : null; | |
| string? headers1 = _options.Headers; | |
| IEnumerable<KeyValuePair<string, string>>? headers = headers1 != null | |
| ? (headers1.Split(',')).Select((Func<string, KeyValuePair<string, string>>)(h => | |
| { | |
| string[] strArray = h.Trim().Split(':', 2); | |
| if (strArray.Length < 2) | |
| throw new ArgumentException($"Invalid header format: '{h}'. Expected 'key:value'."); | |
| return new KeyValuePair<string, string>(strArray[0].Trim(), strArray[1].Trim()); | |
| })) | |
| : null; |
🤖 Prompt for AI Agents
In `@SW.Bitween.NativeAdapters/HttpHandler/HttpHandler.cs` around lines 129 - 136,
The header parsing currently uses h.Split(':') and accesses strArray[1], which
throws on malformed entries and truncates values containing ':'; update the
logic in HttpHandler.cs (the headers1/headers creation) to call h.Split(new[] {
':' }, 2) (or Split(':', 2)) so it only splits into two parts, then check
parts.Length == 2 before creating the KeyValuePair, Trim() both key and value,
and skip or log any malformed entries instead of indexing out of range; ensure
behavior when _options.Headers is null remains unchanged.
| private void WriteValue(JsonWriter writer, object? value) | ||
| { | ||
| if (value == null) | ||
| { | ||
| writer.WriteNull(); | ||
| return; | ||
| } | ||
|
|
||
| var t = JToken.FromObject(value); | ||
| switch (t.Type) | ||
| { | ||
| case JTokenType.Object: | ||
| this.WriteObject(writer, value); | ||
| break; | ||
| case JTokenType.Array: | ||
| this.WriteArray(writer, value); | ||
| break; | ||
| default: | ||
| writer.WriteValue(value); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
JToken.FromObject(value) in WriteValue may cause performance overhead or recursion.
Line 21 creates a full JToken just to inspect the type, then discards it. For large/deep objects this is wasteful. More critically, if the serializer has this converter registered globally, JToken.FromObject(value) could re-enter the converter, causing a stack overflow.
Consider checking value is IDictionary<string, object> and value is IEnumerable<object> directly instead of materializing a JToken.
♻️ Proposed refactor
private void WriteValue(JsonWriter writer, object? value)
{
if (value == null)
{
writer.WriteNull();
return;
}
- var t = JToken.FromObject(value);
- switch (t.Type)
- {
- case JTokenType.Object:
- this.WriteObject(writer, value);
- break;
- case JTokenType.Array:
- this.WriteArray(writer, value);
- break;
- default:
- writer.WriteValue(value);
- break;
- }
+ if (value is IDictionary<string, object>)
+ this.WriteObject(writer, value);
+ else if (value is IEnumerable<object>)
+ this.WriteArray(writer, value);
+ else
+ writer.WriteValue(value);
}🤖 Prompt for AI Agents
In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs` around lines 13 -
34, The WriteValue method currently materializes a JToken via
JToken.FromObject(value) which is wasteful and may re-enter the converter;
replace that logic with direct runtime type checks: if value is
IDictionary<string, object> (or IDictionary) call WriteObject(writer, value),
else if value is IEnumerable and not a string (e.g. value is IEnumerable
enumerable && !(value is string)) call WriteArray(writer, value), otherwise call
writer.WriteValue(value); update the branching in WriteValue (refer to
WriteValue, WriteObject, WriteArray) to use these checks and remove the
JToken.FromObject usage to prevent recursion and improve performance.
| private void WriteObject(JsonWriter writer, object value) | ||
| { | ||
| writer.WriteStartObject(); | ||
| var obj = value as IDictionary<string, object>; | ||
| if (obj != null) | ||
| { | ||
| foreach (var kvp in obj) | ||
| { | ||
| writer.WritePropertyName(kvp.Key); | ||
| this.WriteValue(writer, kvp.Value); | ||
| } | ||
| } | ||
| writer.WriteEndObject(); | ||
| } |
There was a problem hiding this comment.
WriteObject silently writes empty {} for non-dictionary objects.
If JToken.FromObject(value) returns JTokenType.Object but the value is not IDictionary<string, object> (e.g., a plain POCO), the cast on line 39 yields null, and lines 38/48 still emit {} — silently losing all data.
Consider either falling back to serializer.Serialize(writer, value) for non-dictionary objects, or throwing to surface the problem early.
🤖 Prompt for AI Agents
In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs` around lines 36 -
49, The WriteObject method currently emits an empty object for non-IDictionary
values; update WriteObject (in DictionaryConverter.cs) so that after casting to
IDictionary<string, object> it falls back for non-dictionary types by delegating
to JsonSerializer.Serialize(writer, value) (or throw a descriptive
JsonSerializationException if you prefer fail-fast) instead of silently writing
{}. Ensure the chosen approach integrates with the existing
WriteValue/WriteObject flow (add a JsonSerializer parameter or call
JsonSerializer.CreateDefault() as appropriate) and include a clear error message
if you opt to throw.
| private object ReadArray(JsonReader reader) | ||
| { | ||
| IList<object> list = new List<object>(); | ||
|
|
||
| while (reader.Read()) | ||
| { | ||
| switch (reader.TokenType) | ||
| { | ||
| case JsonToken.Comment: | ||
| break; | ||
| case JsonToken.EndArray: | ||
| return list; | ||
| default: | ||
| var v = ReadValue(reader); | ||
| if (v != null) | ||
| list.Add(v); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| throw new JsonSerializationException("Unexpected end when reading IDictionary<string, object>"); | ||
| } |
There was a problem hiding this comment.
ReadArray silently drops JSON null values, altering semantics.
Lines 112-114: when ReadValue returns null (e.g., for a JSON null token), the value is silently skipped. This means [1, null, 3] deserializes to [1, 3]. The same issue exists in ReadObject (lines 139-141) — a property with an explicit null value is dropped entirely.
If this is intentional, add a comment documenting the behavior. Otherwise, you should store nulls to preserve JSON fidelity.
🐛 Proposed fix to preserve nulls
In ReadArray:
default:
var v = ReadValue(reader);
- if (v != null)
- list.Add(v);
+ list.Add(v);
break;In ReadObject:
var v = ReadValue(reader);
- if (v != null)
- obj[propertyName] = v;
+ obj[propertyName] = v;
break;🤖 Prompt for AI Agents
In `@SW.Bitween.NativeAdapters/Services/DictionaryConverter.cs` around lines 99 -
120, ReadArray and ReadObject currently drop entries when ReadValue returns null
(e.g., JSON null), which loses fidelity; update both methods (ReadArray and
ReadObject) so that when ReadValue returns null you explicitly add/store a null
value (e.g., add null to the IList<object> in ReadArray and set the dictionary
value to null in ReadObject) instead of skipping, or if the omission was
intentional add a clear comment explaining the behavior; ensure you reference
the existing ReadValue call sites in ReadArray and the property-assignment block
in ReadObject and adjust them to preserve nulls.
AhmadRAbuhussein
closed this pull request by merging all changes
into
releases/r8.0
in
1a3e39f
2 participants
Summary by CodeRabbit
Release Notes