-
Notifications
You must be signed in to change notification settings - Fork 573
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.
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
Modernize StripeConfiguration #1485
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,117 +7,107 @@ namespace Stripe | |
using Newtonsoft.Json; | ||
using Stripe.Infrastructure; | ||
|
||
/// <summary> | ||
/// Global configuration class for Stripe.net settings. | ||
/// </summary> | ||
public static class StripeConfiguration | ||
{ | ||
private static string stripeApiVersion = "2018-11-08"; | ||
|
||
private static string apiKey; | ||
private static string apiBase; | ||
private static string connectBase; | ||
private static string filesBase; | ||
private static JsonSerializerSettings serializerSettings = DefaultSerializerSettings(); | ||
|
||
static StripeConfiguration() | ||
{ | ||
StripeNetVersion = new AssemblyName(typeof(Requestor).GetTypeInfo().Assembly.FullName).Version.ToString(3); | ||
} | ||
|
||
public static string StripeApiVersion | ||
{ | ||
get { return stripeApiVersion; } | ||
set { stripeApiVersion = value; } | ||
} | ||
/// <summary>API version used by Stripe.net.</summary> | ||
public static string ApiVersion => "2018-11-08"; | ||
|
||
/// <summary> | ||
/// Gets or sets the settings used for deserializing JSON objects returned by Stripe's API. | ||
/// It is highly recommended you do not change these settings, as doing so can produce | ||
/// unexpected results. If you do change these settings, make sure that | ||
/// <see cref="Stripe.Infrastructure.StripeObjectConverter"/> is among the converters, | ||
/// otherwise the library will no longer be able to deserialize polymorphic resources | ||
/// represented by interfaces (e.g. <see cref="IPaymentSource"/>). | ||
/// </summary> | ||
public static JsonSerializerSettings SerializerSettings | ||
{ | ||
get { return serializerSettings; } | ||
set { serializerSettings = value; } | ||
} | ||
/// <summary>Default base URL for Stripe's API.</summary> | ||
public static string DefaultApiBase => "https://api.stripe.com"; | ||
|
||
public static string StripeNetVersion { get; } | ||
/// <summary>Default base URL for Stripe's OAuth API.</summary> | ||
public static string DefaultConnectBase => "https://connect.stripe.com"; | ||
|
||
/// <summary> | ||
/// This option allows you to provide your own HttpMessageHandler. Useful with Android/iPhone. | ||
/// </summary> | ||
public static HttpMessageHandler HttpMessageHandler { get; set; } | ||
/// <summary>Default base URL for Stripe's Files API.</summary> | ||
public static string DefaultFilesBase => "https://files.stripe.com"; | ||
|
||
/// <summary>Default timespan before the request times out.</summary> | ||
public static TimeSpan DefaultHttpTimeout => new TimeSpan(80 * TimeSpan.TicksPerSecond); | ||
|
||
public static TimeSpan? HttpTimeSpan { get; set; } | ||
/// <summary>Gets or sets the base URL for Stripe's API.</summary> | ||
public static string ApiBase { get; set; } = DefaultApiBase; | ||
|
||
internal static string GetApiKey() | ||
#if NET45 || NETSTANDARD2_0 | ||
/// <summary>Gets or sets the API key.</summary> | ||
/// <remarks> | ||
/// You can also set the API key using the <c>StripeApiKey</c> key in | ||
/// <see cref="System.Configuration.ConfigurationManager.AppSettings"/>. | ||
/// </remarks> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel weird about preprocessor commands + comments. Isn't it better to document it globally? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that we can't link to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah I see what you mean. Ugh. Sounds good then! |
||
#else | ||
/// <summary>Gets or sets the API key.</summary> | ||
#endif | ||
public static string ApiKey | ||
{ | ||
#if NET45 | ||
if (string.IsNullOrEmpty(apiKey)) | ||
get | ||
{ | ||
apiKey = System.Configuration.ConfigurationManager.AppSettings["StripeApiKey"]; | ||
} | ||
#if NET45 || NETSTANDARD2_0 | ||
if (string.IsNullOrEmpty(apiKey)) | ||
{ | ||
apiKey = System.Configuration.ConfigurationManager.AppSettings["StripeApiKey"]; | ||
} | ||
#endif | ||
return apiKey; | ||
} | ||
|
||
return apiKey; | ||
} | ||
|
||
public static void SetApiKey(string newApiKey) | ||
{ | ||
apiKey = newApiKey; | ||
} | ||
|
||
internal static string GetApiBase() | ||
{ | ||
if (string.IsNullOrEmpty(apiBase)) | ||
set | ||
{ | ||
apiBase = Urls.DefaultBaseUrl; | ||
apiKey = value; | ||
} | ||
|
||
return apiBase; | ||
} | ||
|
||
public static void SetApiBase(string baseUrl) | ||
{ | ||
apiBase = baseUrl; | ||
} | ||
/// <summary>Gets or sets the base URL for Stripe's OAuth API.</summary> | ||
public static string ConnectBase { get; set; } = DefaultConnectBase; | ||
|
||
internal static string GetConnectBase() | ||
{ | ||
if (string.IsNullOrEmpty(connectBase)) | ||
{ | ||
connectBase = Urls.DefaultBaseConnectUrl; | ||
} | ||
/// <summary>Gets or sets the base URL for Stripe's Files API.</summary> | ||
public static string FilesBase { get; set; } = DefaultFilesBase; | ||
|
||
return connectBase; | ||
} | ||
/// <summary>Gets or sets a custom <see cref="HttpMessageHandler"/>.</summary> | ||
public static HttpMessageHandler HttpMessageHandler { get; set; } | ||
|
||
public static void SetConnectBase(string baseUrl) | ||
{ | ||
connectBase = baseUrl; | ||
} | ||
/// <summary>Gets or sets the timespan to wait before the request times out.</summary> | ||
public static TimeSpan HttpTimeout { get; set; } = DefaultHttpTimeout; | ||
|
||
internal static string GetFilesBase() | ||
{ | ||
if (string.IsNullOrEmpty(filesBase)) | ||
{ | ||
filesBase = Urls.DefaultBaseFilesUrl; | ||
} | ||
/// <summary> | ||
/// Gets or sets the settings used for deserializing JSON objects returned by Stripe's API. | ||
/// It is highly recommended you do not change these settings, as doing so can produce | ||
/// unexpected results. If you do change these settings, make sure that | ||
/// <see cref="Stripe.Infrastructure.StripeObjectConverter"/> is among the converters, | ||
/// otherwise Stripe.net will no longer be able to deserialize polymorphic resources | ||
/// represented by interfaces (e.g. <see cref="IPaymentSource"/>). | ||
/// </summary> | ||
public static JsonSerializerSettings SerializerSettings { get; set; } = DefaultSerializerSettings(); | ||
|
||
return filesBase; | ||
} | ||
/// <summary>Gets the version of the Stripe.net client library.</summary> | ||
public static string StripeNetVersion { get; } | ||
|
||
public static void SetFilesBase(string baseUrl) | ||
/// <summary> | ||
/// Gets or sets the timespan to wait before the request times out. | ||
/// This property is deprecated and will be removed in a future version, please use the | ||
/// <see cref="HttpTimeout"/> property instead. | ||
/// </summary> | ||
// TODO: remove this property in a future major version | ||
[Obsolete("Use StripeConfiguration.HttpTimeout instead.")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're already merging in a major version. Why not just do it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. |
||
public static TimeSpan? HttpTimeSpan | ||
{ | ||
filesBase = baseUrl; | ||
get { return HttpTimeout; } | ||
set { HttpTimeout = value ?? DefaultHttpTimeout; } | ||
} | ||
|
||
/// <summary> | ||
/// Returns a new instance of <see cref="Newtonsoft.Json.JsonSerializerSettings"/> with | ||
/// the default settings used by Stripe.net. | ||
/// </summary> | ||
/// <returns>A <see cref="Newtonsoft.Json.JsonSerializerSettings"/> instance</returns> | ||
/// <returns>A <see cref="Newtonsoft.Json.JsonSerializerSettings"/> instance.</returns> | ||
public static JsonSerializerSettings DefaultSerializerSettings() | ||
{ | ||
return new JsonSerializerSettings | ||
|
@@ -129,5 +119,55 @@ public static JsonSerializerSettings DefaultSerializerSettings() | |
DateParseHandling = DateParseHandling.None, | ||
}; | ||
} | ||
|
||
// TODO: remove everything below this in a future major version | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, you're already planning version 23, why not do it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. |
||
|
||
/// <summary> | ||
/// Sets the base URL.for Stripe's API. | ||
/// This method is deprecated and will be removed in a future version, please use the | ||
/// <see cref="ApiBase"/> property setter instead. | ||
/// </summary> | ||
/// <param name="baseUrl">Base URL.for Stripe's API.</param> | ||
[Obsolete("Use StripeConfiguration.ApiBase getter instead.")] | ||
public static void SetApiBase(string baseUrl) | ||
{ | ||
ApiBase = baseUrl; | ||
} | ||
|
||
/// <summary> | ||
/// Sets the API key. | ||
/// This method is deprecated and will be removed in a future version, please use the | ||
/// <see cref="ApiKey"/> property setter instead. | ||
/// </summary> | ||
/// <param name="newApiKey">API key.</param> | ||
[Obsolete("Use StripeConfiguration.ApiKey setter instead.")] | ||
public static void SetApiKey(string newApiKey) | ||
{ | ||
ApiKey = newApiKey; | ||
} | ||
|
||
/// <summary> | ||
/// Sets the base URL.for Stripe's OAuth API. | ||
/// This method is deprecated and will be removed in a future version, please use the | ||
/// <see cref="ConnectBase"/> property setter instead. | ||
/// </summary> | ||
/// <param name="baseUrl">Base URL.for Stripe's OAuth API.</param> | ||
[Obsolete("Use StripeConfiguration.ConnectBase setter instead.")] | ||
public static void SetConnectBase(string baseUrl) | ||
{ | ||
ConnectBase = baseUrl; | ||
} | ||
|
||
/// <summary> | ||
/// Sets the base URL.for Stripe's Files API. | ||
/// This method is deprecated and will be removed in a future version, please use the | ||
/// <see cref="FilesBase"/> property setter instead. | ||
/// </summary> | ||
/// <param name="baseUrl">Base URL.for Stripe's Files API.</param> | ||
[Obsolete("Use StripeConfiguration.FilesBase setter instead.")] | ||
public static void SetFilesBase(string baseUrl) | ||
{ | ||
FilesBase = baseUrl; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,10 +20,7 @@ static Requestor() | |
? new HttpClient(StripeConfiguration.HttpMessageHandler) | ||
: new HttpClient(); | ||
|
||
if (StripeConfiguration.HttpTimeSpan.HasValue) | ||
{ | ||
HttpClient.Timeout = StripeConfiguration.HttpTimeSpan.Value; | ||
} | ||
HttpClient.Timeout = StripeConfiguration.HttpTimeout; | ||
} | ||
|
||
internal static HttpClient HttpClient { get; private set; } | ||
|
@@ -120,7 +117,7 @@ private static StripeResponse ExecuteRequest(HttpRequestMessage requestMessage) | |
|
||
internal static HttpRequestMessage GetRequestMessage(string url, HttpMethod method, RequestOptions requestOptions) | ||
{ | ||
requestOptions.ApiKey = requestOptions.ApiKey ?? StripeConfiguration.GetApiKey(); | ||
requestOptions.ApiKey = requestOptions.ApiKey ?? StripeConfiguration.ApiKey; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay minor but need to flag: there's no thread safety and such, which I think is okay, but removing a getter prevents from ever adding this right? Though I guess it still has a getter/setter associated with it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow. The only thing that changed with regards to the global API key is that Changing the global API key was never thread-safe. Users that want to use different API keys in a multi-threaded environment must set the API key per request via |
||
|
||
#if NET45 | ||
ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls12 | SecurityProtocolType.Tls11 | SecurityProtocolType.Tls; | ||
|
@@ -148,7 +145,7 @@ internal static HttpRequestMessage GetRequestMessage(string url, HttpMethod meth | |
} | ||
else | ||
{ | ||
request.Headers.Add("Stripe-Version", StripeConfiguration.StripeApiVersion); | ||
request.Headers.Add("Stripe-Version", StripeConfiguration.ApiVersion); | ||
} | ||
|
||
var client = new Client(request); | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this remove
SetApiKey
entirely? Isn't this a breaking change? Or are we just making this cleaner but still supporting both (flagging now since it's the first thing in the review)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention was to avoid a breaking change by keeping the old methods around, but marking them as
Obsolete
to encourage users to upgrade to the new way of setting these values, and actually remove the methods in a future major version. This would make the upgrade a bit less painful for users.I don't feel too strongly about it, if you'd rather we drop the old methods immediately that's fine by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I think that's fine and a nicer experience for sure