Use Guard Class to improve code legibility and avoid lines #339

davidrevoledo · 2018-11-02T00:03:55Z

Description

To be in the same page as others azure libraries it'd be good to avoid lines to improve legibility by checking pre null conditions in a easy way.

This is only a proposition as Guard class seems to do the same as ExceptionUtilities so probabily that code can be there instead or Move the code in Guard to Fx.

Open to suggestions to change it or feel free to close if this don't follow the package guidelines.

All using are in order BTW

This checklist is used to make sure that common guidelines for a pull request are followed.

I have read the contribution guidelines.
Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).
If applicable, the public code is properly documented.
Pull request includes test coverage for the included changes.
The code builds without any errors.

davidrevoledo · 2018-11-02T00:04:08Z

src/Microsoft.Azure.EventHubs.Processor/EventProcessorHost.cs

@@ -13,7 +14,7 @@ namespace Microsoft.Azure.EventHubs.Processor
    public sealed class EventProcessorHost
    {
        // A processor host will work on either the token provider or the connection string.
-        ITokenProvider tokenProvider;
+        readonly ITokenProvider tokenProvider;


This can be readonly #Resolved

davidrevoledo · 2018-11-02T00:04:29Z

src/Microsoft.Azure.EventHubs.Processor/EventProcessorHost.cs

@@ -449,7 +448,7 @@ static string CreateHostName(string prefix)
                prefix = "host";
            }

-            return prefix + "-" + Guid.NewGuid().ToString();
+            return prefix + "-" + Guid.NewGuid();


ToString is doing nothing #Resolved

davidrevoledo · 2018-11-02T00:05:00Z

src/Microsoft.Azure.EventHubs/Primitives/ExceptionUtility.cs

@@ -7,10 +7,6 @@ namespace Microsoft.Azure.EventHubs

    class ExceptionUtility
    {
-        internal ExceptionUtility()


will be the default ctor as the class is internal #Resolved

davidrevoledo · 2018-11-08T02:52:10Z

@serkantkaraca any thoughts about it ? #Resolved

serkantkaraca · 2018-11-08T23:34:47Z

I will look into this ASAP. I was waiting for the previous PR to go in first. I wanted to go one by one since we are not able to run tests on your PRs. #Resolved

serkantkaraca · 2018-11-09T21:58:27Z

src/Microsoft.Azure.EventHubs/Primitives/Guard.cs

+    {
+        internal static void ArgumentNotNull(string argumentName, object value)
+        {
+            if (value == null) Fx.Exception.ArgumentNull(argumentName);


if (value == null) Fx.Exception.ArgumentNull(argumentName); [](start = 12, length = 59)

if (value == null) Fx.Exception.ArgumentNull(argumentName); [](start = 12, length = 59)

Add curly brackets. Same for below. #Resolved

serkantkaraca · 2018-11-09T21:59:35Z

src/Microsoft.Azure.EventHubs/Primitives/EventHubsConnectionStringBuilder.cs

-            {
-                throw Fx.Exception.ArgumentNull(nameof(endpointAddress));
-            }
+            Guard.ArgumentNotNull(nameof(endpointAddress), endpointAddress);
            if (string.IsNullOrWhiteSpace(entityPath))


IsNullOrWhiteSpace [](start = 23, length = 18)

IsNullOrWhiteSpace [](start = 23, length = 18)

You can move this type of checks into Guard as well. #Resolved

serkantkaraca · 2018-11-09T22:02:15Z

src/Microsoft.Azure.EventHubs/EventHubClient.cs

@@ -275,11 +267,7 @@ public sealed override async Task CloseAsync()
        /// <seealso cref="PartitionSender.SendAsync(EventData)"/>
        public Task SendAsync(EventData eventData)
        {
-            if (eventData == null)
-            {
-                throw Fx.Exception.ArgumentNull(nameof(eventData));


ArgumentNull [](start = 35, length = 12)

There are many other public API with argument checks. Can you also cover them? #Resolved

davidrevoledo · 2018-11-12T01:21:08Z

@serkantkaraca Done I leave those validations with custom message or something not standard #Resolved

serkantkaraca · 2018-11-12T20:38:04Z

src/Microsoft.Azure.EventHubs/Primitives/Guard.cs

+            }
+        }
+
+        internal static void ArgumentNotNullOrEmpty(string argumentName, string value)


ArgumentNotNullOrEmpty [](start = 29, length = 22)

Can you name this as ArgumentNotNullOrWhiteSpace? #Resolved

serkantkaraca · 2018-11-12T20:48:53Z

namespace Microsoft.Azure.EventHubs

Find where we throw ArgumentNullException. You will find many more places to apply Guard. #Resolved

Refers to: src/Microsoft.Azure.EventHubs/EventHubClient.cs:4 in f7f8039. [](commit_id = f7f8039, deletion_comment = False)

davidrevoledo · 2018-11-12T21:34:39Z

@serkantkaraca yes I can rename it, yeah there are more in the Processor project will change them #Resolved

davidrevoledo · 2018-11-12T22:21:15Z

@serkantkaraca done :) only leaved in both project those wich custom code #Resolved

davidrevoledo · 2018-11-12T22:23:46Z

src/Microsoft.Azure.EventHubs/Primitives/RetryPolicy.cs

            }

            // Flatten AggregateException
-            else if (exception is AggregateException)
+            if (exception is AggregateException)


else will never execute here #Resolved

serkantkaraca · 2018-11-15T01:36:50Z

src/Microsoft.Azure.EventHubs/EventDataDiagnosticExtensions.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.

+using Microsoft.Azure.EventHubs.Primitives;


Move into namespace #Resolved

Damn good catch. thks #Resolved

davidrevoledo · 2018-11-15T15:31:59Z

@serkantkaraca done ! thks

serkantkaraca

Thanks a bunch. All looks OK.

davidrevoledo · 2018-11-15T18:45:08Z

@serkantkaraca Awesome thks to u, glad to help at least a bit.

serkantkaraca · 2018-11-15T19:02:22Z

We have couple test failures with this change. Can you take a look at them?

https://ci.appveyor.com/project/serkantkaraca/azure-event-hubs-dotnet/branch/dev/job/xg88rhe213pgb7qv/tests

davidrevoledo · 2018-11-15T19:45:26Z

@serkantkaraca there is a little bit problem with tests who are expecting ArgumentNullException and ArgumentException.

First I though in throw in the Guard ArgumentNullException if the string is null and ArgumentException wich make sense, but in some tests they are expecting ArgumentException always for null and empty.

What do you want me to do ?

Changes test to use ArgumentNullException or ArgumentException which you consider better.
Throw ArgumentNullException for null and ArgumentException for empty and break tests to except this.
Pass new exception type to guard to customize from the user if throw one or others to don't change any test.

You tell me and I can implement any of them :) or other if you think.

davidrevoledo · 2018-11-15T21:17:25Z

@serkantkaraca they're just 2 tests that need to be changed from one exception to other , the rest are excepting the proper exception.

davidrevoledo · 2018-11-19T17:07:07Z

Ready to change when get how to do it :)

serkantkaraca · 2018-11-19T17:11:40Z

I haven't checked the tests yet. Let me check the failures first. It is better if we can reserve exceptions as is.

* Use Azure Storage Account NameValidator to check LeaseContainerName (#327) * Create EventHubsTimeoutException for consistency (#328) * Mark readonly fields, complete cancellation token and remove useless Where in LINQ (#326) * Implementing Plugins for EventHub (#324) * Implement Plugin to Process each event when client is sending telemetry * Microsoft copyright header * Fix Typo * Changes for #324 (review) * Implement AfterEventsReceive for EventHubsPlugin * Implement Plugin Tests * Sort usings * changes for #324 (comment) * Fix Resources * Changes for #324 (review) * Change for #324 (comment) * Move Using to Namespace block * Copy Plugins for InnerSender in AmqpEventHubClient (#329) * Prevent event data being over writed when multiple plugins called (#330) * Parallelize expired lease check in processor host (#333) * Parallelize expired lease check * - * Remove unit test and rename FirstPlugin (#335) * Using Lazy instead of static initialization for ExceptionUtility (#337) * Using Lazy instead of static initialization * Use default ctor * Complete Missing CancellationToken (#338) * Fix LazyLoad Ctor (#341) * Nullify Task when The Stop is complete (#342) * Nullfy Task when The Stop is complete * Test for Re Register event processor * Reset CancellationTokenSource * Replace Locks in AmqpEventHubClient and Code Clean ups in AmqpEventHubClient (#345) * Replace Double lock patterns by using Lazy * Code CleanUps in AmqpEventHubClient * Check client management address when it is being created * Remove Result for AzureStorageCheckpointLeaseManager GetAllLeases (#346) * Remove Result for async call * Get awaiter get result for GetAllLeases * Remove useless using * Remove useless initializator * Replace Task Run Call * Remove Task Run * Fix Tests Moving Lazy initialization at the top of the ctor (#347) * Max message size is 1MB now. Updating the test accordingly. (#344) * Use Guard Class to improve code legibility and avoid lines (#339) * Using Guard to improve code reading and avoid lines * Using ArgumentNotNullOrEmpty * Complete More validations with Guard * Replace All ArgumentNullException * Fix Namespace * Change a few expecting exceptions (#351) * Code CleanUps in Primitives / General (#350) * EventHubCode Clean ups * Code CleanUps in Primitives * Rename variable * Leave string cast * Some Code Cleanups in Ampq Implementation (#349) * Omit failures in receive pump (#354) * Clean more results in sync context (#348) * Clean more results in sync context * Order using * Changes for #348 (review) * Remove using * Support partition-empty in runtime metrics (#352) * Adding partition IsEmpty support to runtime metrics * Use AMQP client constants * Move to correct name * is_partition_empty is the correct name * Reduce the number of storage calls in lease manager (#357) * Couple improvements in Azure Lease Manager to reduce numberof storage calls. * N/A as partition id * Go with default timeout * Moving to most recent AMQP release * Fix flaky EPH test * Adding 30 seconds default operation timeout back to tests. * Reducing EPH to storage IO calls. * Couple more fixes * . * Set token for owned leases. * Refresh lease before acquiring in processor host. * Fix metada removal order during lease release. * Update lease token only for already running pumps to avoid resetting receiver position data. * FetchAttributesAsync of blob as part of GetAllLeasesAsync() call. * Refresh lease before attempting to steal * Don't retry if we already lost the lease during receiver open. * Don't attempt to steal if owner has changed from the calculation time to refresh time. * - * Partition pump to close when hit ReceiverDisconnectedException since this is not recoverable. * - * Ignore any failure during releasing the lease * Don't update pump token if token is empty * Nullify the owner on the lease in case this host lost it. * Increment ourLeaseCount when a lease is acquired. * Correcting task list * No need to assign pump lease token to downloaded lease. * comment update * comment update * Clear ownership on partial acquisition. * Clear ownership on partial acquisition. * Make sure we don't leave the lease as owned if acquisition failed. * Adding logs to debug lease corruption bug * Adding logs to debug lease corruption bug * Small fix at steal lease check * Protect subject iterator variable during task creation in for loops. * . * Renew lease right after ChangeLease call * Don't create pump if partition expired or already moved to some other host. * Use refreshed lease while creating partition pump. * Remove temporary debug logs. * Addressing SJ's comments * Remove obsolete * Moving AD SDK 4.5 and Xamarin.iOS10 support (#365) * Making SystemProperties public addressing testability issues. (#332) * Provide batch object's event data enumerator publicly. (#356) * Bumping up the SDK version (#366) * ServiceFabricProcessor preview (#262) This is the code that built and released as preview version 0.5.2 https://www.nuget.org/packages/Microsoft.Azure.EventHubs.ServiceFabricProcessor/0.5.2 At the time it couldn't be merged with dev due to test issues from unrelated work, so we did the release from the SFprocessor branch. Those issues have been resolved, and we expect that future preview releases will come from dev branch. * Client changes to support SFP (#367) Three changes in the client needed to support SFP or SFP testing: 1) A previous PR added the ability to set EventData.SystemProperties, but it is not much use without the ability to create a new SystemPropertiesCollection instance. SFP testing does not need to set individual values on a SystemPropertiesCollection, just create new instances with values that do not change after creation time, so I added a public constructor which sets all the values. 2) SFP was previously creating EventHubClients with connection strings, but there is no string syntax for setting the operation timeout, and the message pump feature on PartitionReceiver uses the operation timeout. The easiest way to let SFP set the operation timeout is to make public the existing EventHubClient.Create call which takes a ConnectionStringBuilder. 3) Originally, SFP was a friend assembly of the client, the same as EPH, but it was decided that that was not the best design. Supporting the EnableReceiverRuntimeMetric option requires the ability to create new ReceiverRuntimeInformation instances (made constructor public) and update the values from a received EventData. Copying the values in SFP code would require making a bunch of properties on EventData public get, and the corresponding properties on ReceiverRuntimeInformation public set. Instead, I added an Update method which takes an EventData and performs the copy within the client assembly so no visibility change is required. Also modified the EPH code to use the new Update method. * delaysign=false

davidrevoledo commented Nov 2, 2018

View reviewed changes

serkantkaraca reviewed Nov 9, 2018

View reviewed changes

davidrevoledo force-pushed the dev-usingGuard branch from 89d2a6b to 496ec88 Compare November 12, 2018 01:03

davidrevoledo force-pushed the dev-usingGuard branch 2 times, most recently from dddd079 to 0cb9fc2 Compare November 12, 2018 19:13

David Revoledo added 3 commits November 12, 2018 17:14

Using Guard to improve code reading and avoid lines

dd40880

Using ArgumentNotNullOrEmpty

20b8cca

Complete More validations with Guard

f7f8039

davidrevoledo force-pushed the dev-usingGuard branch from 0cb9fc2 to f7f8039 Compare November 12, 2018 20:14

serkantkaraca reviewed Nov 12, 2018

View reviewed changes

Replace All ArgumentNullException

4c1a32c

davidrevoledo commented Nov 12, 2018

View reviewed changes

Merge branch 'dev' into dev-usingGuard

55c0001

serkantkaraca reviewed Nov 15, 2018

View reviewed changes

Fix Namespace

c01b375

serkantkaraca approved these changes Nov 15, 2018

View reviewed changes

serkantkaraca merged commit 27aa79c into Azure:dev Nov 15, 2018

davidrevoledo deleted the dev-usingGuard branch November 15, 2018 18:45

davidrevoledo mentioned this pull request Nov 19, 2018

Fix Exception Type expected in Tests #351

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Guard Class to improve code legibility and avoid lines #339

Use Guard Class to improve code legibility and avoid lines #339

davidrevoledo commented Nov 2, 2018

davidrevoledo Nov 2, 2018 •

edited by serkantkaraca

Loading

davidrevoledo Nov 2, 2018 •

edited by serkantkaraca

Loading

davidrevoledo Nov 2, 2018 •

edited by serkantkaraca

Loading

davidrevoledo commented Nov 8, 2018 •

edited by serkantkaraca

Loading

serkantkaraca commented Nov 8, 2018 •

edited

Loading

serkantkaraca Nov 9, 2018 •

edited

Loading

serkantkaraca Nov 9, 2018 •

edited

Loading

serkantkaraca Nov 9, 2018 •

edited

Loading

davidrevoledo commented Nov 12, 2018 •

edited by serkantkaraca

Loading

serkantkaraca Nov 12, 2018 •

edited

Loading

serkantkaraca commented Nov 12, 2018 •

edited

Loading

davidrevoledo commented Nov 12, 2018 •

edited by serkantkaraca

Loading

davidrevoledo commented Nov 12, 2018 •

edited by serkantkaraca

Loading

davidrevoledo Nov 12, 2018 •

edited by serkantkaraca

Loading

serkantkaraca Nov 15, 2018 •

edited

Loading

davidrevoledo Nov 15, 2018 •

edited by serkantkaraca

Loading

davidrevoledo commented Nov 15, 2018

serkantkaraca left a comment

davidrevoledo commented Nov 15, 2018

serkantkaraca commented Nov 15, 2018

davidrevoledo commented Nov 15, 2018

davidrevoledo commented Nov 15, 2018 •

edited

Loading

davidrevoledo commented Nov 19, 2018

serkantkaraca commented Nov 19, 2018

Use Guard Class to improve code legibility and avoid lines #339

Use Guard Class to improve code legibility and avoid lines #339

Conversation

davidrevoledo commented Nov 2, 2018

Description

davidrevoledo Nov 2, 2018 • edited by serkantkaraca Loading

Choose a reason for hiding this comment

davidrevoledo Nov 2, 2018 • edited by serkantkaraca Loading

Choose a reason for hiding this comment

davidrevoledo Nov 2, 2018 • edited by serkantkaraca Loading

Choose a reason for hiding this comment

davidrevoledo commented Nov 8, 2018 • edited by serkantkaraca Loading

serkantkaraca commented Nov 8, 2018 • edited Loading

serkantkaraca Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

serkantkaraca Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

serkantkaraca Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

davidrevoledo commented Nov 12, 2018 • edited by serkantkaraca Loading

serkantkaraca Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

serkantkaraca commented Nov 12, 2018 • edited Loading

davidrevoledo commented Nov 12, 2018 • edited by serkantkaraca Loading

davidrevoledo commented Nov 12, 2018 • edited by serkantkaraca Loading

davidrevoledo Nov 12, 2018 • edited by serkantkaraca Loading

Choose a reason for hiding this comment

serkantkaraca Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

davidrevoledo Nov 15, 2018 • edited by serkantkaraca Loading

Choose a reason for hiding this comment

davidrevoledo commented Nov 15, 2018

serkantkaraca left a comment

Choose a reason for hiding this comment

davidrevoledo commented Nov 15, 2018

serkantkaraca commented Nov 15, 2018

davidrevoledo commented Nov 15, 2018

davidrevoledo commented Nov 15, 2018 • edited Loading

davidrevoledo commented Nov 19, 2018

serkantkaraca commented Nov 19, 2018

davidrevoledo Nov 2, 2018 •

edited by serkantkaraca

Loading

davidrevoledo Nov 2, 2018 •

edited by serkantkaraca

Loading

davidrevoledo Nov 2, 2018 •

edited by serkantkaraca

Loading

davidrevoledo commented Nov 8, 2018 •

edited by serkantkaraca

Loading

serkantkaraca commented Nov 8, 2018 •

edited

Loading

serkantkaraca Nov 9, 2018 •

edited

Loading

serkantkaraca Nov 9, 2018 •

edited

Loading

serkantkaraca Nov 9, 2018 •

edited

Loading

davidrevoledo commented Nov 12, 2018 •

edited by serkantkaraca

Loading

serkantkaraca Nov 12, 2018 •

edited

Loading

serkantkaraca commented Nov 12, 2018 •

edited

Loading

davidrevoledo commented Nov 12, 2018 •

edited by serkantkaraca

Loading

davidrevoledo commented Nov 12, 2018 •

edited by serkantkaraca

Loading

davidrevoledo Nov 12, 2018 •

edited by serkantkaraca

Loading

serkantkaraca Nov 15, 2018 •

edited

Loading

davidrevoledo Nov 15, 2018 •

edited by serkantkaraca

Loading

davidrevoledo commented Nov 15, 2018 •

edited

Loading