Add convenience layer to generated code for Event Grid Track 2 #14442

Soreloser2 · 2020-08-25T23:03:57Z

This PR is based on #14440, and new commits start at 51b5908.

Create Clients and public event model classes on top of generated system events.
Write tests for publish and consumption of events
Document with readme, samples, javadocs

Autorest regenerated using up to date version for track 2 pom file incorporated to jacoco and dependencies for module handled issue around capitalization of etag discrepancy fixed no test coverage yet

Autogenerate EventGrid track 2 classes using the swagger. All files except EventGridPublisherImplTests.java, pom.xml, and the swagger readme.md are autogenerated.

Convenience layer for EventGrid Track 2 on top of autogenerated code.

Add a few missing javadoc comments and remove a few comments from private methods.

test and pom edits mostly

Rename methods and use StepVerifier instead of blocking

Drop explicit support for non-JSON data. Modify/fix tests slightly.

SharedAccessSignature -> SAS, refactor methods and classes

Soreloser2 · 2020-09-02T23:54:32Z

/azp run

azure-pipelines · 2020-09-02T23:54:38Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

Soreloser2 · 2020-09-02T23:55:39Z

/azp run java - eventgrid - ci, java - eventgrid - ci (Build Test Windows - Java 11)

azure-pipelines · 2020-09-02T23:55:50Z

Azure Pipelines successfully started running 1 pipeline(s).

conforms to live test expected custom event properties

srnagar · 2020-09-04T20:46:30Z

sdk/eventgrid/azure-messaging-eventgrid/README.md

+In order to start sending events, we need an `EventGridPublisherClient`. Here is code to 
+create the synchronous and the asynchronous versions. Note that a shared access signature can
+be used instead of a key in any of these samples by calling the `sharedAccessSignatureCredential` 
+method instead of `keyCredential'. 


Suggested change

method instead of `keyCredential'.

method instead of `keyCredential`.

srnagar · 2020-09-04T20:49:13Z

...id/azure-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridEvent.java

+    public <T> T getData(Class<T> clazz, JsonSerializer dataDeserializer) {
+        if (!parsed) {
+            // data was set instead of parsed, throw exception because we don't know how the data relates to clazz
+            throw new IllegalStateException("This method should only be called on events created through the parse method");
+        }
+
+        return dataDeserializer.deserialize(new ByteArrayInputStream((byte[]) this.event.getData()),
+            TypeReference.createInstance(clazz));
+    }


We should consider adding async overloads for these since dataDeserializer supports async deserialization.

srnagar · 2020-09-04T20:52:06Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+    public EventGridServiceVersion getServiceVersion() {
+        return EventGridServiceVersion.V2018_01_01;
+    }


Instead of hardcoding the version here, the builder (since the builder should have a setter for service version) should pass this to the constructor and the client should use that as an instance variable that will be returned here.

srnagar · 2020-09-04T20:54:08Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+        return withContext(context -> publishEvents(events, context));
+    }
+
+    Mono<Void> publishEvents(Iterable<EventGridEvent> events, Context context) {


Why is this called publish? It's better to use the same terminology even for non-public methods.

srnagar · 2020-09-04T20:55:39Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+        List<com.azure.messaging.eventgrid.implementation.models.CloudEvent> implList = new ArrayList<>();
+        for (CloudEvent event : events) {
+            implList.add(event.toImpl());
+        }


Converting to inner types should happen lazily when the Mono is subscribed. Do the same at other places too where the list operations are performed eagerly.

srnagar · 2020-09-04T21:00:06Z

...g-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherClientBuilder.java

+     *
+     * @return the builder itself.
+     */
+    public EventGridPublisherClientBuilder keyCredential(AzureKeyCredential credential) {


The method should be named credential to be consistent with other SDK builders.

srnagar · 2020-09-04T21:01:13Z

...-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridSasCredential.java

+            return String.format("%s&%s=%s", unsignedSas, signKey, encodedSignature);
+
+        } catch (NoSuchAlgorithmException | UnsupportedEncodingException | InvalidKeyException e) {
+            e.printStackTrace();


Use logs instead of e.printStackTrace().

srnagar · 2020-09-04T21:01:29Z

...-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridSasCredential.java

+     */
+    public EventGridSasCredential(String sas) {
+        if (CoreUtils.isNullOrEmpty(sas)) {
+            throw new IllegalArgumentException("the access signature cannot be null or empty");


Log and throw.

srnagar · 2020-09-04T21:02:15Z

...ure-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/SystemEventMappings.java

+import java.util.Locale;
+import java.util.Map;
+
+public final class SystemEventMappings {


Missing javadoc.

srnagar · 2020-09-04T21:03:02Z

...grid/azure-messaging-eventgrid/src/samples/java/com/azure/messaging/eventgrid/ClassTime.java

+import java.time.ZoneOffset;
+import java.util.Random;
+
+public class ClassTime {


Add javadocs for all samples to explain what the sample does.

srnagar

Overall, looks great and it's good to see all design guidelines being followed! Thanks @Soreloser2

bsiegel · 2020-09-04T22:25:22Z

...ntgrid/azure-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/CloudEvent.java

+import com.azure.core.annotation.Fluent;
+import com.azure.core.serializer.json.jackson.JacksonJsonSerializerBuilder;
+import com.azure.core.util.CoreUtils;
+import com.azure.core.util.serializer.*;


Generally I think we try to avoid * imports, I know I had to go dive into IntelliJ settings to make it stop doing this by default:

Settings > Editor > Code Style > Java > set both count to use import with '*' settings to 99.

bsiegel · 2020-09-04T22:27:43Z

...ntgrid/azure-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/CloudEvent.java

+    public Object getData() {
+        if (!parsed) {
+            // data was set instead of parsed, throw error
+            throw new IllegalStateException("This method should only be called on events created through the parse method");


I think Srikanta already called this out, but instead of throwing directly, we should get a ClientLogger instance and call throw logger.logExceptionAsWarning/Error(...) instead.

bsiegel · 2020-09-04T22:33:23Z

...grid/azure-messaging-eventgrid/src/samples/java/com/azure/messaging/eventgrid/ClassTime.java

@@ -0,0 +1,55 @@
+package com.azure.messaging.eventgrid;


Make sure all source files have the copyright & license header.

Variety of changes, major ones include: - Async methods on generic getData - Lazy list creation for send methods - Credential methods both renamed to `credential` - Logging errors instead of throwing - Additional missing javadoc comments

Soreloser2 · 2020-09-04T23:06:53Z

Thanks for all the feedback, @srnagar and @bsiegel! I'm pretty sure I addressed all the comments, but let me know if there's anything else.

srnagar

LGTM! Some minor cosmetic changes required before merging.

srnagar · 2020-09-04T23:16:49Z

...ntgrid/azure-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/CloudEvent.java

+import java.io.ByteArrayOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.time.OffsetDateTime;
+import java.util.*;


Enumerate all imports instead of using *. Wonder why this is happening on generated code though. Is this how autorest generated? If that's the case, let me know. I will take a look.

I believe this has to do with my IntelliJ settings, because this is code written over the autogenerated model in implementation. Brandon mentioned this in a different file as well but my settings should be fixed now.

srnagar · 2020-09-04T23:19:35Z

...id/azure-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridEvent.java

+    public <T> Mono<T> getDataAsync(Class<T> clazz, JsonSerializer dataDeserializer) {
+        if (!parsed) {
+            // data was set instead of parsed, throw exception because we don't know how the data relates to clazz
+            throw logger.logExceptionAsError(new IllegalStateException(


For async methods, exceptions should be passed through the publisher. Use FluxUtil.monoError().

srnagar · 2020-09-04T23:25:34Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+        return Flux.fromIterable(events)
+            .collectList()
+            .flatMap(list -> this.impl.publishCustomEventEventsAsync(this.hostname, list, context));


Unfortunately, we'll have to iterate through the collection twice - once to convert Iterable to list and then when serializing. This impacts performance. Not sure, if we can avoid this without changing the API to take a List instead.

srnagar · 2020-09-04T23:26:59Z

...-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridSasCredential.java

+            return String.format("%s&%s=%s", unsignedSas, signKey, encodedSignature);
+
+        } catch (NoSuchAlgorithmException | UnsupportedEncodingException | InvalidKeyException e) {
+            throw new RuntimeException(logger.logThrowableAsError(e));


Suggested change

throw new RuntimeException(logger.logThrowableAsError(e));

throw logger.logThrowableAsError(new RuntimeException(e));

srnagar · 2020-09-04T23:29:30Z

...-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridSasCredential.java

+        if (CoreUtils.isNullOrEmpty(sas)) {
+            throw logger.logExceptionAsError(new IllegalArgumentException("the access signature cannot be null or empty"));
+        }


https://azure.github.io/azure-sdk/java_implementation.html#java-errors-system-errors
Split the exception into NPE and IllegalArgumentException when the input is null and empty respectively.

srnagar · 2020-09-04T23:32:43Z

...g-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherClientBuilder.java

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for
+// license information.


License headers should be the following in all Java files:
https://docs.opensource.microsoft.com/content/releasing/copyright-headers.html#java

// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License.

mostly minor changes, except a small license change to all source files, including generated ones.

Soreloser2 added 5 commits July 1, 2020 14:32

Regenerate SDK from Autorest v4

ff8f427

Autorest regenerated using up to date version for track 2 pom file incorporated to jacoco and dependencies for module handled issue around capitalization of etag discrepancy fixed no test coverage yet

Regenerate SDK from Autorest v4

12bf02a

Autorest regenerated using up to date version for track 2 pom file incorporated to jacoco and dependencies for module handled issue around capitalization of etag discrepancy fixed no test coverage yet

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java

9327f4c

EventGrid Track 2 Autogenerated

c24cc38

Autogenerate EventGrid track 2 classes using the swagger. All files except EventGridPublisherImplTests.java, pom.xml, and the swagger readme.md are autogenerated.

EventGrid Track 2 written code

51b5908

Convenience layer for EventGrid Track 2 on top of autogenerated code.

Soreloser2 requested review from bsiegel and srnagar August 25, 2020 23:03

Soreloser2 added 4 commits August 26, 2020 11:36

Add Javadoc comments

d76ebbe

Add a few missing javadoc comments and remove a few comments from private methods.

Merge in changes from autogenerated PR

153a22e

test and pom edits mostly

Fix tests

b247b8e

Rename methods and use StepVerifier instead of blocking

Add Changelog

ecb6fe2

Soreloser2 force-pushed the tsdahl/written branch from 52dcaf8 to ecb6fe2 Compare August 27, 2020 18:38

Soreloser2 self-assigned this Aug 28, 2020

Soreloser2 added Client This issue points to a problem in the data-plane of the library. Event Grid labels Aug 28, 2020

Soreloser2 added this to the [2020] September milestone Aug 28, 2020

Soreloser2 added 2 commits August 28, 2020 12:10

Rework CloudEvent serialization

339867e

Drop explicit support for non-JSON data. Modify/fix tests slightly.

Abbreviate SAS

3927dfb

SharedAccessSignature -> SAS, refactor methods and classes

Soreloser2 marked this pull request as ready for review August 28, 2020 23:11

Soreloser2 requested review from alzimmermsft, anuchandy, conniey, danieljurek, hemanttanwar, jianghaolu, JimSuplizio, JonathanGiles, joshfree, mitchdenny and samvaity as code owners August 28, 2020 23:11

bsiegel requested review from srnagar and removed request for mitchdenny, joshfree, JonathanGiles, anuchandy, danieljurek, jianghaolu, hemanttanwar, weshaggard, conniey, JimSuplizio, samvaity, alzimmermsft and srnagar September 3, 2020 21:03

Changed CustomEvent test

1d5ef37

conforms to live test expected custom event properties

srnagar reviewed Sep 4, 2020

View reviewed changes

bsiegel reviewed Sep 4, 2020

View reviewed changes

Address review comments

f5ae4ad

Variety of changes, major ones include: - Async methods on generic getData - Lazy list creation for send methods - Credential methods both renamed to `credential` - Logging errors instead of throwing - Additional missing javadoc comments

srnagar approved these changes Sep 4, 2020

View reviewed changes

Final set of review comments

655825b

mostly minor changes, except a small license change to all source files, including generated ones.

Soreloser2 merged commit 18528bd into Azure:master Sep 5, 2020

Soreloser2 deleted the tsdahl/written branch September 5, 2020 00:42

weshaggard mentioned this pull request Sep 10, 2020

Switch eventgrid ci to client instead of data #15064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add convenience layer to generated code for Event Grid Track 2 #14442

Add convenience layer to generated code for Event Grid Track 2 #14442

Soreloser2 commented Aug 25, 2020

Soreloser2 commented Sep 2, 2020

azure-pipelines bot commented Sep 2, 2020

Soreloser2 commented Sep 2, 2020

azure-pipelines bot commented Sep 2, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar left a comment

bsiegel Sep 4, 2020

bsiegel Sep 4, 2020

bsiegel Sep 4, 2020

Soreloser2 commented Sep 4, 2020

srnagar left a comment •

edited

Loading

srnagar Sep 4, 2020

Soreloser2 Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

srnagar Sep 4, 2020

	method instead of `keyCredential'.
	method instead of `keyCredential`.

	throw new RuntimeException(logger.logThrowableAsError(e));
	throw logger.logThrowableAsError(new RuntimeException(e));

Add convenience layer to generated code for Event Grid Track 2 #14442

Add convenience layer to generated code for Event Grid Track 2 #14442

Conversation

Soreloser2 commented Aug 25, 2020

Soreloser2 commented Sep 2, 2020

azure-pipelines bot commented Sep 2, 2020

Soreloser2 commented Sep 2, 2020

azure-pipelines bot commented Sep 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soreloser2 commented Sep 4, 2020

srnagar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar left a comment •

edited

Loading