Skip to content

fix: Update TwilioRestClient.cs to add overloaded method in rest client #770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AsabuHere merged 6 commits into from
Nov 27, 2024
+37 −4
Merged

fix: Update TwilioRestClient.cs to add overloaded method in rest client #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a7f638f
Update TwilioRestClient.cs to add overloaded method in rest client
AsabuHere Nov 26, 2024
c89e69e
Update TwilioRestClient.cs to make auth strategy a mandatory parameter
AsabuHere Nov 27, 2024
80fc806
moving mandatory params before optional ones
AsabuHere Nov 27, 2024
dae0ef1
Update Twilio.cs reordering the parameters
AsabuHere Nov 27, 2024
88de0b0
Merge branch 'main' into bug_fix_public_oauth
AsabuHere Nov 27, 2024
bf8c2e4
Adding env variables
AsabuHere Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/test-and-deploy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ jobs:
TWILIO_API_SECRET: ${{ secrets.TWILIO_CLUSTER_TEST_API_KEY_SECRET }}
TWILIO_FROM_NUMBER: ${{ secrets.TWILIO_FROM_NUMBER }}
TWILIO_TO_NUMBER: ${{ secrets.TWILIO_TO_NUMBER }}
TWILIO_CLIENT_ID: ${{ secrets.TWILIO_CLIENT_ID }}
TWILIO_CLIENT_SECRET: ${{ secrets.TWILIO_CLIENT_SECRET }}
TWILIO_MESSAGE_SID: ${{ secrets.TWILIO_MESSAGE_SID }}
run: |
dotnet tool install --global dotnet-sonarscanner
make cover
Expand Down
34 changes: 32 additions & 2 deletions src/Twilio/Clients/TwilioRestClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,38 @@ public TwilioRestClient(
string accountSid = null,
string region = null,
HttpClient httpClient = null,
string edge = null,
AuthStrategy authstrategy = null
string edge = null
)
{
_username = username;
_password = password;

AccountSid = accountSid ?? username;
HttpClient = httpClient ?? DefaultClient();

Region = region;
Edge = edge;
}

/// <summary>
/// Constructor for a TwilioRestClient
/// </summary>
///
/// <param name="username">username for requests</param>
/// <param name="password">password for requests</param>
/// <param name="accountSid">account sid to make requests for</param>
/// <param name="region">region to make requests for</param>
/// <param name="httpClient">http client used to make the requests</param>
/// <param name="edge">edge to make requests for</param>
/// <param name="authstrategy">authentication strategy that will be used to make requests for</param>
public TwilioRestClient(
string username,
string password,
AuthStrategy authstrategy,
string accountSid = null,
string region = null,
HttpClient httpClient = null,
string edge = null
)
{
_username = username;
Expand Down
4 changes: 2 additions & 2 deletions src/Twilio/Twilio.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public static ITwilioRestClient GetNoAuthRestClient()
}

AuthStrategy noauthstrategy = new NoAuthStrategy();
_noAuthRestClient = new TwilioRestClient(_username, _password, accountSid: _accountSid, region: _region, edge: _edge, authstrategy: noauthstrategy)
_noAuthRestClient = new TwilioRestClient(_username, _password, authstrategy: noauthstrategy, accountSid: _accountSid, region: _region, edge: _edge)
Copy link
Copy Markdown
Contributor

@tiwarishubham635 tiwarishubham635 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the case that order in which the params are passed, is considered? If that's the case, we should add authStrategy as the last param since it won't break any current user who is passing region or edge specifically.

Copy link
Copy Markdown
Contributor Author

@AsabuHere AsabuHere Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order in which parameters passed is not considered if we mention the name of the parameters.
There was an issue reported because we added a new optional parameter auth strategy

In this PR, this was implemented as two different overloaded methods. Restored the old method as it was. Added a new method with authstrategy a mandatory parameter. If we add auth strategy as an optional parameter, these two method calls become ambigous and throws error while compiling

So I added auth strategy as mandatory parameter. Mandatory parameters has to be appeared before optional ones

Copy link
Copy Markdown
Contributor

@tiwarishubham635 tiwarishubham635 Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that makes sense. Thanks!

Copy link
Copy Markdown

@williamdenton williamdenton Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey - this change in order absolutlely matters and you have broken your aspnet library with this change

https://github.com/twilio-labs/twilio-aspnet/blob/e13d459b812b6a305144a2677e06e02d36c41083/src/Twilio.AspNet.Core/TwilioClientDependencyInjectionExtensions.cs#L182-L204

this change is not binary compatible with the prior release and there is now MissingMethodExceptions occurring because of this.

Named parameters (and optional parameters) are only syntactic sugar. they are baked into the caller and the compiler will reorder and substitute defaults in the calling library.

You need to urgently release an updated version of the aspnet library that is compiled against this version of the package. Or preferably fix the constructor on this package to make it compatible again.

Also changing a public constructor on a public class is a breaking change, thats a semver major version, this could have been easily avoided by not changing the order.

Copy link
Copy Markdown

@williamdenton williamdenton Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linking to the issue #768

{
LogLevel = _logLevel
};
Expand Down Expand Up @@ -240,7 +240,7 @@ public static ITwilioRestClient GetRestClient()
}
else if(_credentialProvider != null){
AuthStrategy authstrategy = _credentialProvider.ToAuthStrategy();
_restClient = new TwilioRestClient(_username, _password, accountSid: _accountSid, region: _region, edge: _edge, authstrategy: _credentialProvider.ToAuthStrategy())
_restClient = new TwilioRestClient(_username, _password, authstrategy: _credentialProvider.ToAuthStrategy(), accountSid: _accountSid, region: _region, edge: _edge)
{
LogLevel = _logLevel
};
Expand Down