Skip to content
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

Initial checkin for WinMD WindowsAzure API #3

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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 microsoft-azure-servicelayer/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
*.suo
TestApp/
microsoft-azure-servicelayer-pr.sln
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
*.suo
bin/
obj/

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
Copy link

Choose a reason for hiding this comment

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

Would like to consult with Ted here on the headers, for C# I almost always prefer proper two-slash comments including for license headers.

But if there's team precedence here, I'm OK with this.

Copy link
Author

Choose a reason for hiding this comment

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

These were copied from a java file. I'll check with Ted tomorrow if he has C#-specific headers.

Copy link
Author

Choose a reason for hiding this comment

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

All files have been updated with the proper heading.

* Copyright 2012 Microsoft Corporation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.WindowsAzure.ServiceLayer
{
class AzureServiceException: Exception
Copy link

Choose a reason for hiding this comment

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

Would nice to be specific with public/internal/private. Also mark it abstract if it isn't designed to be directly instantiated.

Copy link
Author

Choose a reason for hiding this comment

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

Is there a class that cannot be instantiated that is not marked as abstract? I usually do that, but I may have missed something.

Copy link
Member

Choose a reason for hiding this comment

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

We should be consistent with Azure vs. WindowsAzure in our naming.

Copy link
Author

Choose a reason for hiding this comment

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

Renamed to WindowsAzureServiceException. However, I am not sure if I want/need to keep this class - need to figure out whole error handling story in WinMD files. It turned out that you cannot declare public exception classes in WinMD libraries.

Copy link
Member

Choose a reason for hiding this comment

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

You should update your pull request (just checkin again to the same branch you submitted the pull from and github will propagate so we see updates in the PR).

{
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright 2012 Microsoft Corporation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

using System;
using System.Collections.Generic;
using System.Net.Http;
using System.Threading.Tasks;

namespace Microsoft.WindowsAzure.ServiceLayer
{
class HttpErrorHandler: MessageProcessingHandler
Copy link

Choose a reason for hiding this comment

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

Same feedback here.

Copy link
Author

Choose a reason for hiding this comment

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

All classes and class members have been updated with explicitly specified visibility attributes.

{
/// <summary>
/// Constructor.
/// </summary>
internal HttpErrorHandler()
: base()
{
}

/// <summary>
/// Constructor.
/// </summary>
/// <param name="innerHandler">Inner HTTP handler</param>
Copy link

Choose a reason for hiding this comment

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

It is another small nit, but consistency in XML comments is good... so please end with a period here: 'Inner HTTP handler.'

Copy link
Author

Choose a reason for hiding this comment

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

Updated punctuation on all param descriptions and return values.

internal HttpErrorHandler(HttpMessageHandler innerHandler)
: base(innerHandler)
{
}

/// <summary>
/// Processes outhoing HTTP requests.
Copy link

Choose a reason for hiding this comment

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

Spelling error.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

/// </summary>
/// <param name="request">Request</param>
/// <param name="cancellationToken">Cancellation token</param>
Copy link

Choose a reason for hiding this comment

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

This one is fun, there are 2 correct ways to spell Cancellation/Cancelation.

Should check what the .NET framework naming guidelines say. I honestly don't remember. Windows phone got this wrong and so they have both in the API..

Copy link
Author

Choose a reason for hiding this comment

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

That name was autogenerated by VS when I told it to implement abstract parent class.

Copy link

Choose a reason for hiding this comment

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

Great, thanks. That's the spelling we will stick with!

/// <returns>Processed HTTP request</returns>
protected override HttpRequestMessage ProcessRequest(HttpRequestMessage request, System.Threading.CancellationToken cancellationToken)
Copy link

Choose a reason for hiding this comment

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

Consider using the System.Threading namespace and removing the explicit declaration.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed everywhere in the project. That namespace is not added by default, and when VS generates method, it uses full class name.

{
// We're not interested in outgoing requests; do nothing.
return request;
}

/// <summary>
/// Processes incoming HTTP responses.
/// </summary>
/// <param name="response">HTTP response</param>
/// <param name="cancellationToken">Cancellation token</param>
/// <returns>Processed HTTP response</returns>
protected override HttpResponseMessage ProcessResponse(HttpResponseMessage response, System.Threading.CancellationToken cancellationToken)
{
if (!response.IsSuccessStatusCode)
throw new AzureServiceException();
Copy link

Choose a reason for hiding this comment

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

My preference is to always have even simple logic blocks contained with curly braces.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is the right place or way to throw this exception:

  1. What does the stack trace look like when something blows up in the pipeline? If it's really confusing to a user where the error came from, we might think about at least rethrowing from within the service itself to rebase the call stack.
  2. Some methods return specific codes in the 400s to indicate what type of failure. Furthermore, most failing status codes will come with an XML error payload that describes exactly what went wrong (which is probably what we should use as the message of the exception).

Copy link
Author

Choose a reason for hiding this comment

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

Error handling in asynchronous methods is an interesting story. You'll get an AggregateException when calling Task.Wait or when obtaining Task.Result property. That exception provides you access to all exceptions that happened while executing the call.

Note that you cannot define your own public exceptions.

Copy link
Member

Choose a reason for hiding this comment

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

You can definitely define your own exceptions - it just might not be possible to pass them across WinRT boundaries. In that case, if you want to detect them later on, you should define your own and then a WrapExceptionForWinRT static method that transforms the exception into something you can rethrow with a useful callstack.

Copy link
Author

Choose a reason for hiding this comment

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

OK, thanks for the info. Unless you object, I am going to submit the code as is - and take care of the static method in one of the following checkins. The main idea, however, still remains the same - HTTP errors get translated into exceptions by a handler; when exception occurs, further tasks in the chain are not executed.

return response;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright 2012 Microsoft Corporation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Microsoft.WindowsAzure.ServiceLayer
{
/// <summary>
/// Helper class for processing HTTP query strings.
/// </summary>
class HttpQuery
Copy link
Member

Choose a reason for hiding this comment

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

The name makes me think this class does something different. Perhaps HttpQueryStringParameters would be more descriptive.

Copy link
Author

Choose a reason for hiding this comment

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

Renamed into HttpQueryStringParser.

{
Dictionary<string, string> _values = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
Copy link

Choose a reason for hiding this comment

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

Explicit private indicator would be nice for the field.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed for all class members.


/// <summary>
/// Constructor.
/// </summary>
/// <param name="queryString">Query string</param>
internal HttpQuery(string queryString)
{
string[] pairs = queryString.Split('&');
Copy link
Member

Choose a reason for hiding this comment

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

Has this already been URL decoded? If not, your logic below could get a little messy. There's an equivalent of http://msdn.microsoft.com/en-us/library/adwtk1fy.aspx tucked away somewhere in the core libraries in the new platform. You might also need to trim a leading ? depending on where you get the querystring from.

Copy link
Author

Choose a reason for hiding this comment

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

It has been. The class is used in a single place - to extract WRAP token details from the response of authentication purpose.

The class for encoding/decoding is called System.Net.WebUtility.


foreach (string pair in pairs)
{
int pos = pair.IndexOf('=');
string name = pair.Substring(0, pos);
Copy link

Choose a reason for hiding this comment

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

For robustness, might want to check pos >= 0 - or at least validate what will happen if an invalid query string is passed in. Might be worth using a more robust query string parsing library - we have some in ASP.NET and other parts of C# / libraries that we could consider using.

Copy link
Author

Choose a reason for hiding this comment

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

I was looking for classes that could parse HTTP queries and found one only in ASP.Net. I did not want, however, to add a dependency on that library only to use their parser.

Missing characters in the query string would mean malformed input. Processing that would require us to abort execution anyway. If we don't check return values, we'll get something like an "index out of range" exception. If we do - we can throw something nicer. However, I am not sure it's worth the effort.

string value = pair.Substring(pos + 1);
_values.Add(name, value);
}
}

/// <summary>
/// Gets parameter by name.
/// </summary>
/// <param name="parameterName">Parameter name</param>
/// <returns>Parameter value</returns>
internal string this[string parameterName] { get { return _values[parameterName]; } }
Copy link

Choose a reason for hiding this comment

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

My preference would be to break this out into many lines to improve readability.

Copy link
Author

Choose a reason for hiding this comment

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

I usually do not split simple properties that just return values, into multiple lines. Keeping such properties on a single line results in denser code, which is not a bad thing, too.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a fan of multiple lines here too - mostly because that's the default StyleCop/SourceAnalysis setting and then everything looks delightfully uniform.

Copy link
Author

Choose a reason for hiding this comment

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

Code analysis in VS IDE with default settings does not complain about single-line properties. It did complain about double closing pattern for streams/readers, which I fixed.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" Condition="Exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props')" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<ProductVersion>8.0.30703</ProductVersion>
<SchemaVersion>2.0</SchemaVersion>
<ProjectGuid>{53C097E2-7384-446B-836B-A7910993091E}</ProjectGuid>
<OutputType>winmdobj</OutputType>
<AppDesignerFolder>Properties</AppDesignerFolder>
<RootNamespace>Microsoft.WindowsAzure.ServiceLayer</RootNamespace>
<AssemblyName>Microsoft.WindowsAzure.ServiceLayer</AssemblyName>
<DefaultLanguage>en-US</DefaultLanguage>
<FileAlignment>512</FileAlignment>
<ProjectTypeGuids>{BC8A1FFA-BEE3-4634-8014-F334798102B3};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
<DebugSymbols>true</DebugSymbols>
<DebugType>full</DebugType>
<Optimize>false</Optimize>
<OutputPath>bin\Debug\</OutputPath>
<DefineConstants>DEBUG;TRACE;NETFX_CORE</DefineConstants>
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
<DebugType>pdbonly</DebugType>
<Optimize>true</Optimize>
<OutputPath>bin\Release\</OutputPath>
<DefineConstants>TRACE;NETFX_CORE</DefineConstants>
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|ARM'">
<DebugSymbols>true</DebugSymbols>
<OutputPath>bin\ARM\Debug\</OutputPath>
<DefineConstants>DEBUG;TRACE;NETFX_CORE</DefineConstants>
<NoWarn>;2008</NoWarn>
<DebugType>full</DebugType>
<PlatformTarget>ARM</PlatformTarget>
<UseVSHostingProcess>false</UseVSHostingProcess>
<ErrorReport>prompt</ErrorReport>
<CodeAnalysisRuleSet>ExpressRules.ruleset</CodeAnalysisRuleSet>
<Prefer32Bit>true</Prefer32Bit>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|ARM'">
<OutputPath>bin\ARM\Release\</OutputPath>
<DefineConstants>TRACE;NETFX_CORE</DefineConstants>
<Optimize>true</Optimize>
<NoWarn>;2008</NoWarn>
<DebugType>pdbonly</DebugType>
<PlatformTarget>ARM</PlatformTarget>
<UseVSHostingProcess>false</UseVSHostingProcess>
<ErrorReport>prompt</ErrorReport>
<CodeAnalysisRuleSet>ExpressRules.ruleset</CodeAnalysisRuleSet>
<Prefer32Bit>true</Prefer32Bit>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'">
<DebugSymbols>true</DebugSymbols>
<OutputPath>bin\x64\Debug\</OutputPath>
<DefineConstants>DEBUG;TRACE;NETFX_CORE</DefineConstants>
<NoWarn>;2008</NoWarn>
<DebugType>full</DebugType>
<PlatformTarget>x64</PlatformTarget>
<UseVSHostingProcess>false</UseVSHostingProcess>
<ErrorReport>prompt</ErrorReport>
<CodeAnalysisRuleSet>ExpressRules.ruleset</CodeAnalysisRuleSet>
<Prefer32Bit>true</Prefer32Bit>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'">
<OutputPath>bin\x64\Release\</OutputPath>
<DefineConstants>TRACE;NETFX_CORE</DefineConstants>
<Optimize>true</Optimize>
<NoWarn>;2008</NoWarn>
<DebugType>pdbonly</DebugType>
<PlatformTarget>x64</PlatformTarget>
<UseVSHostingProcess>false</UseVSHostingProcess>
<ErrorReport>prompt</ErrorReport>
<CodeAnalysisRuleSet>ExpressRules.ruleset</CodeAnalysisRuleSet>
<Prefer32Bit>true</Prefer32Bit>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x86'">
<DebugSymbols>true</DebugSymbols>
<OutputPath>bin\x86\Debug\</OutputPath>
<DefineConstants>DEBUG;TRACE;NETFX_CORE</DefineConstants>
<NoWarn>;2008</NoWarn>
<DebugType>full</DebugType>
<PlatformTarget>x86</PlatformTarget>
<UseVSHostingProcess>false</UseVSHostingProcess>
<ErrorReport>prompt</ErrorReport>
<CodeAnalysisRuleSet>ExpressRules.ruleset</CodeAnalysisRuleSet>
<Prefer32Bit>true</Prefer32Bit>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x86'">
<OutputPath>bin\x86\Release\</OutputPath>
<DefineConstants>TRACE;NETFX_CORE</DefineConstants>
<Optimize>true</Optimize>
<NoWarn>;2008</NoWarn>
<DebugType>pdbonly</DebugType>
<PlatformTarget>x86</PlatformTarget>
<UseVSHostingProcess>false</UseVSHostingProcess>
<ErrorReport>prompt</ErrorReport>
<CodeAnalysisRuleSet>ExpressRules.ruleset</CodeAnalysisRuleSet>
<Prefer32Bit>true</Prefer32Bit>
</PropertyGroup>
<ItemGroup>
<!-- A reference to the entire .Net Framework and Windows SDK are automatically included -->
</ItemGroup>
<ItemGroup>
<Compile Include="AzureServiceException.cs" />
<Compile Include="HttpErrorHandler.cs" />
<Compile Include="HttpQuery.cs" />
<Compile Include="ServiceBus\ServiceBusRestProxy.cs" />
<Compile Include="ServiceBus\WrapAuthenticationHandler.cs" />
<Compile Include="ServiceBus\WrapToken.cs" />
<Compile Include="ServiceBus\IServiceBusService.cs" />
<Compile Include="Properties\AssemblyInfo.cs" />
<Compile Include="ServiceBus\QueueInfo.cs" />
<Compile Include="ServiceBus\QueueSettings.cs" />
<Compile Include="SerializationHelper.cs" />
<Compile Include="ServiceBus\ServiceBusService.cs" />
<Compile Include="ServiceBus\ServiceConfiguration.cs" />
</ItemGroup>
<PropertyGroup Condition=" '$(VisualStudioVersion)' == '' ">
<VisualStudioVersion>11.0</VisualStudioVersion>
</PropertyGroup>
<PropertyGroup>
<StartupObject />
</PropertyGroup>
<Import Project="$(MSBuildExtensionsPath)\Microsoft\WindowsXaml\v$(VisualStudioVersion)\Microsoft.Windows.UI.Xaml.CSharp.targets" />
<!-- To modify your build process, add your task inside one of the targets below and uncomment it.
Other similar extension points exist, see Microsoft.Common.targets.
<Target Name="BeforeBuild">
</Target>
<Target Name="AfterBuild">
</Target>
-->
</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System.Reflection;
Copy link

Choose a reason for hiding this comment

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

Need the copyright header in here as well.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

// General Information about an assembly is controlled through the following
// set of attributes. Change these attribute values to modify the information
// associated with an assembly.
[assembly: AssemblyTitle("Microsoft.WindowsAzure.ServiceLayer")]
[assembly: AssemblyDescription("")]
[assembly: AssemblyConfiguration("")]
[assembly: AssemblyCompany("Microsoft")]
[assembly: AssemblyProduct("Microsoft.WindowsAzure.ServiceLayer")]
[assembly: AssemblyCopyright("Copyright © Microsoft 2012")]
Copy link
Member

Choose a reason for hiding this comment

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

Should both this line and the one two above use "Microsoft Corporation" instead?

Copy link
Author

Choose a reason for hiding this comment

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

This file was automatically generated and conveniently placed into a subdirectory, where it missed my attention. Fixed.

[assembly: AssemblyTrademark("")]
[assembly: AssemblyCulture("")]

// Version information for an assembly consists of the following four values:
//
// Major Version
// Minor Version
// Build Number
// Revision
//
// You can specify all the values or you can default the Build and Revision Numbers
// by using the '*' as shown below:
// [assembly: AssemblyVersion("1.0.*")]
[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyFileVersion("1.0.0.0")]
[assembly: ComVisible(false)]
Loading