Client API are updated to not require an IP address for transaction reporting #246
Conversation
| @@ -83,6 +90,15 @@ public sealed class TransactionReport | |||
| string? transactionId = null | |||
| ) | |||
| { | |||
| if (ipAddress == null && minfraudId == null && string.IsNullOrEmpty(maxmindId) | |||
There was a problem hiding this comment.
It might be worth checking that minfraudId is not the 0 UUID too. I think this can be done with minfraudId.Empty. Maybe we should only set it if it is non-empty too.
| Assert.Equal(transactionId, report.TransactionId); | ||
|
|
||
| // no identifier supplied | ||
| Assert.Throws<ArgumentException>(() => new TransactionReport(ipAddress: null, tag: tag)); |
There was a problem hiding this comment.
Maybe it would be worth testing the empty string case too?
| public TransactionReport( | ||
| IPAddress ipAddress, | ||
| IPAddress? ipAddress, |
There was a problem hiding this comment.
I think it would be worth adding a new constructor where ipAddress is an optional parameter so that people don't have to specify it every time. We could mark this one as obsolete.
There was a problem hiding this comment.
It is not clear to me how to disambiguate the constructors that way. If I keep the original constructor and add an identical constructor except ipAddress is optional, the compiler will give a "The call is ambiguous between the following methods or properties" error. Even if it could disambiguate it seems like it would give the deprecation error anytime the ipAddress was passed, similar to the following:
# `!` marks a named parameter as required, which are otherwise optional without it
$ raku -e 'multi sub foo(:$ipaddress!, :$tag!) { say "deprecated constructor" }; multi sub foo(:$ipaddress, :$tag) { say "new constructor" }; foo(tag => 42); foo(ipaddress => 42, tag => 42)'
new constructor
deprecated constructor
Is there some pattern for working around this? It sort of seems like what we want is to deprecate passing ipAddress as the first positional parameter.
There was a problem hiding this comment.
Ah, yeah, that is a bit annoying. I was thinking something like:
public TransactionReport(
TransactionReportTag tag,
string? chargebackCode = null,
IPAddress? ipAddress = null,
string? maxmindId = null,
Guid? minfraudId = null,
string? notes = null,
string? transactionId = null
)The constructors aren't necessarily ambiguous, but when the callers specify all of the parameters by name, they are. I think you could work around this by changing the existing constructor to something like:
[Obsolete]
public TransactionReport(
IPAddress? ipAddress,
TransactionReportTag tagObsolete,
string? chargebackCode = null,
string? maxmindId = null,
Guid? minfraudId = null,
string? notes = null,
string? transactionId = null
)It is a bit ugly, but it should solve the issue. Given that we are doing a major version release (or so it seems), we could forgo this, but I think it would be nice to give people a few releases to update their code before dropping the old construcotr.
There was a problem hiding this comment.
If I understand correctly the thought was the ordering of the parameters (which otherwise appear identical minus what I assume is copy-pasta tagObsolete) between the two candidate methods would disambiguate them. I don't think that'll work though:
Type 'TransactionReport' already defines a member called 'TransactionReport' with the same parameter types
It appears C# does not take ordering into account for method signatures 🤔
There was a problem hiding this comment.
Yes, it does take ordering into account if you pass ordered parameters. If you pass parameters by name, it does not take the order into account. That latter is the issue here. tagObsolete was intentional as it changes the named parameters between the two constructors and tag will always be passed. As such, we continue support anyone who was using positional parameters while anyone using the named parameters would use the new constructor.
There was a problem hiding this comment.
Ah sorry, I mistakenly had a third candidate of that function in my code that was causing that error 🤦♂️
| @@ -83,21 +90,33 @@ public sealed class TransactionReport | |||
| string? transactionId = null | |||
| ) | |||
| { | |||
| if (ipAddress == null && (minfraudId == null || minfraudId == Guid.Empty) | |||
| && string.IsNullOrEmpty(maxmindId) && string.IsNullOrEmpty(transactionId)) | |||
There was a problem hiding this comment.
Hmm, this interacts poorly with init properties, but I don't have an immediate solution for that. Although we encourage init properties for minFraud requests, we don't really encourage them for the report transaction API. I suppose we can worry about this in the future.
| } | ||
|
|
||
| /// <summary> | ||
| /// The IP address reported to MaxMind for the transaction. | ||
| /// </summary> | ||
| [JsonPropertyName("ip_address")] | ||
| [JsonConverter(typeof(IPAddressConverter))] | ||
| public IPAddress IPAddress { get; init; } | ||
| public IPAddress? IPAddress { get; init; } |
There was a problem hiding this comment.
Hmm, this seems like a breaking change. I don't know if is likely to cause actual breakage for anyone though. Maybe we should do a major version bump to be safe though.
This marks the ipAddress as optional and adds a deprecated constructor to maintain backwards compatibility with the original.
ugexe
force-pushed
the
nlogan/no-ip-report
branch
from
cdcc722
to
b88a7c7
Compare
No description provided.