jsistudios / simplerestservices Goto Github PK

The assembly should be signed so it has a strong name. Otherwise, the library will be unusable from other libraries which have strong names.

(I already fixed this issue and will send a pull request shortly)

If a service is unreachable (e.g. network cable unplugged, connection timeout, DNS resolution failure, or proxy problems), the library can "silently" fail.

This issue stems from RestServiceBase catching exceptions that is does not truly handle. Properly preserving the error information for handling at a higher level requires the following changes.

• RestServiceBase.ExecuteRequest should rethrow a WebException when the Response property is null.
• RestServiceBase.ExecuteRequest should not attempt to handle exceptions thrown by the callback methods.
• RestServiceBase.BuildWebResponse should throw an exception if the resp argument is null.
• RestServiceBase.BuildWebResponse should not hide exceptions thrown when reading the response stream.

The server functionality is currently undocumented. It should be documented to improve usability or removed to simplify the library if it is no longer used.

One of the arguments to IRetryLogic<T, T2>.Execute is a retry count. If the call to logic fails and the retry count has reached 0, how should the call to Execute behave?

• Should it throw an exception, and if so, which one?
• Should it return a particular value? If so, would it be default(T) or the last (unsuccessful) value returned from logic?

See openstacknetsdk/openstack.net#492 for details.

When RequestSettings.RetryCount is greater than 0, the streaming operation does not reset the input Stream.Position to the original value before retrying the operation. This can lead to data corruption, where a streaming operation completes successfully but did not transfer the full data.

Currently RestServiceBase.BuildWebResponse<T> hides any StringSerializationException that is thrown. These exceptions expose flaws in the calling code's object model, so they should be propagated instead of hidden. The documentation for IRestService needs to be updated to reflect the changes.

If a web exception is caught, and the calling method tries to serialize the error to response a StringSerializationException is thrown

Currently the implementation of RequestRetryLogic.Execute treats retryCount as one less than it actually is. For example, if retryCount is 1 then the operation will not be retried. If retryCount is 2 then the operation is retried only 1 time.

RestServiceBase.Stream does not account for the maxReadLength when making a call to content.Read. When the maxReadLength is less than the length of the stream, and not a multiple of bufferSize, this leaves the stream position at an unexpected offset when the method returns.

REST calls should not be retried unless a retry is explicitly requested as part of the library usage. In other words, the various retryCount arguments should all default to 0.

This is consistent with the RequestSettings.RetryCount property, which already defaults to 0. Existing user code is extremely unlikely to be affected by this change, because all of the normal entry points to this SDK use a RequestSettings object, which specifies the retry count in the RetryCount property.

WebResponseRetryLogic currently executes one extra retry operation than is specified. For example, if retryCount is specified as 1, the call will be attempted a total of 3 times (one original, plus 2 retry operations).

(I already fixed this issue and will send a pull request shortly)

UrlBuilder uses HttpUtility.HtmlEncode for a few things, leading to two problems.

1. The query string is incorrectly constructed when the value contains certain characters such as a space. The code should use HttpUtility.UrlEncode instead.
2. Since HttpUtility.HtmlEncode("?") == "?", the duplicate check when selecting a separator is unnecessary.

The NuGet assembly should include the XML documentation file along with the new assembly.

To ensure that clients are able to reference this project reliably, the assembly versioning needs to be slightly altered.

1. The Build and Revision fields for the AssemblyVersionAttribute should always be 0. This attribute is used for the strong name of the assembly, so by the definition of these fields as indicating a backwards-compatible assembly they MUST never change.
2. The Build and Revision fields for the AssemblyFileVersionAttribute should be updated to indicate the actual release version of the assembly, minus any prerelease version suffix (if applicable).
3. The AssemblyInformationalVersionAttribute should be maintained to indicate the complete version of the assembly, including any prerelease version suffix (if applicable).

The license for the project should be more visible to users looking at the assembly information or the project on GitHub. In addition, since there are no exception to the MIT license for files in the SimpleRESTServices.dll assembly, the license notice could make that more clear without requiring users to look at all the individual source files.