twilio/twilio-csharp

Please restore ordering of "code" parameter in VerificationCheckResource.CreateAsync

kevbry opened this issue · 2 comments

kevbry commented

Issue Summary

5.77.0 was released with an undocumented breaking change to at least the VerificationCheckResource.CreateAsync method. Because of the nature of this change and the way it was implemented, the breakage occurs only at runtime. While the changelog has been updated today to note the breaking change, I don't believe this is sufficient given the nature of the change.

See f853988#diff-d5a51009fb1625214b8a8be7a484667eb349d508b17f895ec8cf0be344b2c0e2 under VerificationCheckResource.cs line 117.

public static async System.Threading.Tasks.Task<VerificationCheckResource> CreateAsync(string pathServiceSid,
  string code,
  string to = null,
  string verificationSid = null,
  string amount = null,
  string payee = null,
  ITwilioRestClient client = null)

changed to

public static async System.Threading.Tasks.Task<VerificationCheckResource> CreateAsync(string pathServiceSid,
  string to = null,
  string verificationSid = null,
  string amount = null,
  string payee = null,
  string code = null,
  ITwilioRestClient client = null)

Note that the "code" parameter changed both from required to optional, and changed relative position. This means that a consumer calling the method as below using a previous version of the library:

var result = await VerificationCheckResource.CreateAsync(SID, code, toAddress, verificationSID)

will not experience a compilation failure (since those positional parameters are still strings), but instead will be passing in values to the wrong parameters of the method and will experience a runtime failure. Even though the twilio samples use named parameters when calling the method, doing so is not a globally common practice and relying on it as a library author should be avoided. Should also note that no mock/stub-based unit test on a consumer side would have caught this since the method still has the same number of parameters and they're all the same types as before.

I don't believe simply updating the changelog is sufficient - either the code parameter should be moved back to its previous position, or the nuget release should be pulled and replaced with a major version rev reflecting that a breaking change to the API has occurred.
https://semver.org/#what-do-i-do-if-i-accidentally-release-a-backwards-incompatible-change-as-a-minor-version

childish-sambino commented

Similar issue to twilio/twilio-php#740

Product team is working on changing the ordering back so that code is the first param after pathServiceSid and it will now be optional (note that this won't revert the PHP change given how required/optional params are passed in differently).

  • 👍1