EventGrid Beta4 by YijunXieMS · Pull Request #18873 · Azure/azure-sdk-for-java

YijunXieMS · 2021-01-29T01:15:25Z

This PR mainly has the API changes for EventGrid beta4:

CloudEvent constructor accept a new parameter "data".
CloudEvent.parse and EventGridEvent.parse are renamed to fromString()
getData() of CloudEvent and EventGridEvent return BinaryData. Removed the generic version of getData that deserializes data into a certain type. Instead, users can use BinaryData to deserialize data.
added isSystemEvent / asSystemEventData to CloudEvent and EventGridEvent. (removed later)
Removed getDataAsync
Added EventGridSasGenerator class. Removed EventGridSasCredential
Renamed sendEvents to sendEventGridEvents
Added sample code for the new APIs
Some other changes.

closes #18281
closes #18276
closes #18275
closes #14926
closes #18427

…lder

…nto eg-beta4

srnagar · 2021-01-29T01:18:29Z

sdk/eventgrid/azure-messaging-eventgrid/pom.xml

+    <dependency>
+      <groupId>com.azure</groupId>
+      <artifactId>azure-storage-queue</artifactId>
+      <version>12.8.0</version>
+      <scope>test</scope>
+    </dependency>


This dependency should be removed.

this is test scope for a sample. The released SDK won't have it.

sdk/eventgrid/azure-messaging-eventgrid/pom.xml

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

...e-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridDeserializer.java

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

...e-messaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridSasGenerator.java

...ng-eventgrid/src/samples/java/com/azure/messaging/eventgrid/samples/ProcessSystemEvents.java

check-enforcer · 2021-02-03T02:06:31Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

hemanttanwar · 2021-02-04T08:22:46Z

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

+     * @param data the payload of this event. Set to null if your event doesn't have the data payload.
+     *             It will be serialized as a String if it's a String, or application/json if it's not a String.
+     */
+    public CloudEvent(String source, String type, Object data) {


Can CloudEvent have no data ? If data is optional, why it is provided in constructor ? User can call setData() if they need to. constructor are normally used for required parameters.

Also Add @throws clause , since it will throw IllegalArgumentException. After that you do not need to additionally explain It can't be null or empty .

Yes a CloudEvent can have no data. This sounds weird but the specification says it's optional.
We still put it in the constructor because we believe 99% of the time a CloudEvent will have data.

hemanttanwar · 2021-02-04T08:24:49Z

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

     */
-    public CloudEvent(String source, String type) {
+    public CloudEvent(String source, String type, byte[] data, String dataContentType) {
+        this(source, type);


null check for data and dataContentType ?

Both data and dataContentType can be null.

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

hemanttanwar · 2021-02-04T08:28:02Z

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

     * @param source a URI identifying the origin of the event.
     * @param type   the type of event, e.g. "Contoso.Items.ItemReceived".
+     * @param data the payload in bytes of this event. It will be serialized to Base64 format.
+     * @param dataContentType the type of the data.


add @throws clause.

hemanttanwar · 2021-02-04T08:29:38Z

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

-        } else if (CoreUtils.isNullOrEmpty(type)) {
+        }
+        if (CoreUtils.isNullOrEmpty(type)) {
            throw logger.logExceptionAsError(new IllegalArgumentException("type cannot be null or empty"));


nit: casing, t in type should be capitol Type can not be null or empty.

I changed to 'type' because the param name is type.

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

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

hemanttanwar · 2021-02-04T08:46:45Z

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

        return Flux.fromIterable(events)
-            .map(EventGridEvent::toImpl)
+            .map(event -> {
+                com.azure.messaging.eventgrid.implementation.models.EventGridEvent internalEvent = event.toImpl();


This logic is at two places (also at line 121) Can we combine it in a function ?

They look the same but one is for EventGridEvent and one for CloudEvent. the two classes don't have a common parent class or interface so I prefer not combining these lines of code.
Good points though.

...essaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherClient.java

srnagar

Overall, looks good. A few minor comments. Just do another pass on improving the javadocs.

sdk/eventgrid/azure-messaging-eventgrid/pom.xml

srnagar · 2021-02-05T00:35:17Z

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

+ * The CloudEvent model. This represents a cloud event as specified by the
+ * <a href="https://github.com/cloudevents/spec/blob/v1.0.1/spec.md">Cloud Native Computing Foundation</a>
+ *
+ * When you send a CloudEvent to an Event Grid Topic, the topic must be configured to receive the CloudEvent schema.


When this is moved to azure-core, we should update this doc to remove Event Grid specific details.

Added to follow-up issue #19014

srnagar · 2021-02-05T00:39:31Z

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

+     *
+     * @throws IllegalArgumentException if subject, eventType or data is {@code null} or empty.
     */
    public EventGridEvent(String subject, String eventType, Object data, String dataVersion) {


Since subject, eventType and dataVersion are required, I would put them as the first 3 params and move data to the end.

I'll need to check with service team why data is optional and dataVersion is mandatory after beta4.
Added to follow-up issue #19014

srnagar · 2021-02-05T00:43:03Z

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

@@ -68,19 +72,28 @@ public EventGridServiceVersion getServiceVersion() {
     * @param events the EventGrid events to publish.
     *
     * @return the completion.


Update this doc - A {@link Mono} that completes when the events are sent to the service.

Thanks for the suggestion. Updated

srnagar · 2021-02-05T00:47:12Z

...-messaging-eventgrid/src/samples/java/com/azure/messaging/eventgrid/samples/models/User.java

+
+package com.azure.messaging.eventgrid.samples.models;
+
+public class User {


Since this is in samples and users would look at this, we should have javadocs here.

Added a javadoc for the class

srnagar · 2021-02-05T00:54:34Z

...id/src/samples/java/com/azure/messaging/eventgrid/samples/PublishEventGridEventsToTopic.java

+
+/**
+ * This sample code shows how to send {@link EventGridEvent}s to an Event Grid Topic that accepts Event Grid event schema.
+ * Refer to https://docs.microsoft.com/en-us/azure/event-grid/event-schema.


Use html tag for the link.

srnagar · 2021-02-05T00:55:16Z

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

+ *
+ * For new customers, {@link CloudEvent} is generally preferred over EventGridEvent because the
+ * <a href="https://docs.microsoft.com/azure/event-grid/cloud-event-schema">CloudEvent schema</a> is supported across
+ * organizations while the <a href="https://docs.microsoft.com/azure/event-grid/cloud-event-schema">EventGridEvent schema</a> is not.


Suggested change

* organizations while the <a href="https://docs.microsoft.com/azure/event-grid/cloud-event-schema">EventGridEvent schema</a> is not.

* organizations while the <a href="https://docs.microsoft.com/azure/event-grid/event-schema">EventGridEvent schema</a> is not.

Good catch. Updated

srnagar · 2021-02-05T00:57:37Z

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

     * @param events the custom events to publish.
     *
     * @return the completion.
+     * @throws NullPointerException if events is {@code null}.


Add more documentation to this as this is an uncommon scenario. Describe when the user can send custom events and what are the prerequisites for sending a custom event.

Added to follow-up issue #19014

srnagar · 2021-02-05T00:58:27Z

...essaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherClient.java

+     * @throws NullPointerException if events is {@code null}.
     */
    @ServiceMethod(returns = ReturnType.SINGLE)
    public Response<Void> sendCustomEventsWithResponse(Iterable<Object> events, Context context) {


More docs for custom event APIs.

Added to follow-up issue #19014

srnagar · 2021-02-05T01:00:56Z

...essaging-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherClient.java

    /**
     * Publishes the given EventGrid events to the given topic or domain.
     * @param events the EventGrid events to publish.
+     * @throws NullPointerException if events is {@code null}.


All service methods (on how to use the API), client builder and clients (how to instantiate a client) should have code snippets in javadoc.

Added to follow-up issue #19014

srnagar

LGTM!

YijunXieMS added 30 commits December 18, 2020 13:15

Remove param serializerAdapter from EventGridPublisher client and bui…

845c48c

…lder

Remove param serializerAdapter from EventGridPublisher client and bui…

f1526c6

…lder

add unreleased azure-core 1.12.0-beta.1

0385cb4

Use azure-core AzureSasCredential and remove EventGridSasCredential.java

40e9840

Merge branch 'eg-beta4' of github.com:YijunXieMS/azure-sdk-for-java i…

192e460

…nto eg-beta4

Move createSas from EventGridPublisherClient to EventGridSasCreator

f1c35cc

Add EventParser for user study

a9173b7

Remove "opens com.azure.messaging.eventgrid.implementation;"

d723767

Use BinaryData for getData() and add getSystemEventData()

4112c73

Add new sample code

39b84c3

Add sample code that processes system events

c85cbcc

Remove canonicalizeEventType(String eventType)

0a72e1e

Use Object instead of BinaryData for CloudEvent and EventGridEvent data.

7c9993f

Remove redundant ()

9a92db7

Add missing JavaDoc

de6e0e8

Add clientOptions

a5406ce

Update from api review

23cac4d

Update azure-core dependency to 1.12.0

1f00f9a

Reformat json file

b1e4fea

Update Readme

5a48fa9

Fix livetests

d26442f

Add BinaryData, Domain and system events in README

82caee9

Rename SharedAccessSignature to Sas

c8aa65d

Add exception handling for deserialization

ace222d

Small fixes

5d46e5f

Small fixes

9ef1c08

comment wording adjustment

9339140

Add more description about event schema

609eaeb

Rename sendEvents to sendEventGridEvents

f3017d5

Correct command line that queries key

29d896b

YijunXieMS added 3 commits January 29, 2021 09:55

Use abs path for links

446d63a

Update fromString comment

6ae4a50

Remove /en-us/ in links

bb2292d

srnagar reviewed Feb 1, 2021

View reviewed changes

YijunXieMS added 6 commits February 1, 2021 13:18

change setData accessibility from public to package level

4dd155d

Adjust readme and sample

56c13f4

Remove dependency on com.azure.core.serializer.json.jackson

4b02945

Remove or hide isSystemEvent and asSystemEventData

2697863

Remove the hidden asSystemEventData

7ee1c9c

Remove dependencies on commons-io and azure core jackson

fb1e6fb

Merge branch 'master' into eg-beta4

ed102d9

YijunXieMS marked this pull request as ready for review February 3, 2021 05:37

YijunXieMS added 5 commits February 3, 2021 13:54

Remove usage of InputStream.readAll() because of Java 8.

f445374

Revamp CloudEvent constructor

34c0a14

Small updates

cad6ae8

Explain data serialization in CloudEvent constructor

45c6320

System event data wording change.

47c056e

hemanttanwar reviewed Feb 4, 2021

View reviewed changes

YijunXieMS added 3 commits February 4, 2021 09:50

Add data content type when sending event data in bytes

d7c677e

Add throws NPE in method doc

894e692

Small fix

87b1782

srnagar reviewed Feb 5, 2021

View reviewed changes

YijunXieMS added 2 commits February 5, 2021 10:28

Docs update

0607065

remove commons-io

a2db759

YijunXieMS mentioned this pull request Feb 5, 2021

EventGrid Post beta4 release follow-up #19014

Closed

Use <a></a> instead of bare url

266df0c

srnagar approved these changes Feb 5, 2021

View reviewed changes

YijunXieMS merged commit 815526b into Azure:master Feb 5, 2021

vcolin7 mentioned this pull request Feb 8, 2021

[FEATURE REQ] Support SAS credentials in Azure.Data.Tables #19060

Closed


		package com.azure.messaging.eventgrid.samples.models;

		public class User {

	* organizations while the <a href="https://docs.microsoft.com/azure/event-grid/cloud-event-schema">EventGridEvent schema</a> is not.
	* organizations while the <a href="https://docs.microsoft.com/azure/event-grid/event-schema">EventGridEvent schema</a> is not.

Conversation

YijunXieMS commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

check-enforcer bot commented Feb 3, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srnagar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YijunXieMS commented Jan 29, 2021 •

edited

Loading