Skip to content

Binarydata api update - Default json serializer #16319

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.

Sign up for GitHub

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

Jump to bottom
Merged
hemanttanwar merged 25 commits into from
Oct 23, 2020
Merged

Binarydata api update - Default json serializer #16319

hemanttanwar merged 25 commits into from
Oct 23, 2020

Conversation

@hemanttanwar
Copy link
Contributor

@hemanttanwar hemanttanwar commented Oct 14, 2020 •
edited
Loading

@ghost ghost added the Azure.Core azure-core label Oct 14, 2020
alzimmermsft
alzimmermsft reviewed Oct 14, 2020
View reviewed changes
JonathanGiles
JonathanGiles requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

JavaDocs need a pass over to be made more clear.

alzimmermsft
alzimmermsft reviewed Oct 20, 2020
View reviewed changes
sdk/core/azure-core-experimental/src/main/java/com/azure/core/experimental/util/BinaryData.java
*
* @see JsonSerializer
*/
public static BinaryData fromObject(Object data) {
Copy link
Member

@alzimmermsft alzimmermsft Oct 20, 2020

Choose a reason for hiding this comment

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

Given this API is converting into JSON we may want to call it fromObjectJson.

Copy link
Contributor Author

@hemanttanwar hemanttanwar Oct 21, 2020

Choose a reason for hiding this comment

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

If we change it, we need to sync with .Net. I copied this API name from them.

sdk/core/azure-core-experimental/src/main/java/com/azure/core/experimental/util/BinaryData.java
* @param <T> Generic type that the data is deserialized into.
* @return The {@link Object} of given type after deserializing the bytes.
*/
public <T> T toObject(Class<T> clazz) {
Copy link
Member

@alzimmermsft alzimmermsft Oct 20, 2020

Choose a reason for hiding this comment

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

Should we rename this API to something JSON specific? toJsonObject?

@JonathanGiles @srnagar

Copy link
Contributor Author

@hemanttanwar hemanttanwar Oct 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This might force us, in future, to add other API for other formats.

Copy link
Member

@srnagar srnagar Oct 21, 2020

Choose a reason for hiding this comment

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

JsonObject has a different meaning. So, I think toObject() is fine as we will return a strongly-typed user-defined object here.

JonathanGiles
JonathanGiles requested changes Oct 21, 2020
View reviewed changes
sdk/core/azure-core-experimental/src/main/java/com/azure/core/experimental/util/BinaryData.java Outdated
* can be created from {@link InputStream}, {@link Flux} of {@link ByteBuffer}, {@link String}, {@link Object} and byte
* array. The data is collected from provided sources and stored into a byte array.
* This class is an abstraction over many different ways that binary data can be represented. The data represented by
* {@link BinaryData} is immutable. The {@link BinaryData} can be created from {@link InputStream}, {@link Flux} of
Copy link
Member

@JonathanGiles JonathanGiles Oct 21, 2020

Choose a reason for hiding this comment

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

"The data represented by {@link BinaryData} is immutable" is an odd statement to make? It is taken into BinaryData and presumably copied, but when given back to the user is mutable again. Perhaps clarify what you mean by this statement.

sdk/core/azure-core-experimental/src/main/java/com/azure/core/experimental/util/BinaryData.java
} catch (IOException ex) {
throw LOGGER.logExceptionAsError(new UncheckedIOException(ex));
}
}
Copy link
Member

@JonathanGiles JonathanGiles Oct 21, 2020

Choose a reason for hiding this comment

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

Are we not able to handle this in a more efficient manner? As it stands this means that the entire input stream is read into a buffer, converted into a byte array, and then copied into a new byte array? At the very least you can avoid the byte array copy.

Copy link
Contributor Author

@hemanttanwar hemanttanwar Oct 22, 2020

Choose a reason for hiding this comment

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

Removed the last byte array and same optimization in other from..() API as well.

sdk/core/azure-core-experimental/src/main/java/com/azure/core/experimental/util/BinaryData.java
/**
* Serialize the given {@link Object} into {@link BinaryData} using json serializer which is available on classpath.
* The serializer on classpath must implement {@link JsonSerializer} interface. If the given Object is {@code null},
* an empty {@link BinaryData} will be returned.
Copy link
Member

@JonathanGiles JonathanGiles Oct 21, 2020

Choose a reason for hiding this comment

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

This documentation should link through an aka.ms link to the appropriate json serializer documentation (which will soon move to docs.microsoft.com, which is why in the meanwhile we will use an aka.ms link)

Copy link
Contributor Author

@hemanttanwar hemanttanwar Oct 22, 2020

Choose a reason for hiding this comment

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

Added this link https://aka.ms/azsdk/java/wiki/serialization

JonathanGiles
JonathanGiles reviewed Oct 22, 2020
View reviewed changes
Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

keep iterating over the javadoc to fix typos, make messaging clearer, etc.

@hemanttanwar hemanttanwar mentioned this pull request Oct 22, 2020
Closed
JonathanGiles
JonathanGiles approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

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

It looks better. I still think the JavaDoc could be improved to give more clarity around json serializer though.

@hemanttanwar
Copy link
Contributor Author

hemanttanwar commented Oct 23, 2020

/check-enforcer reset

@hemanttanwar
Copy link
Contributor Author

hemanttanwar commented Oct 23, 2020

/check-enforcer run

@hemanttanwar
Copy link
Contributor Author

hemanttanwar commented Oct 23, 2020

/check-enforcer evaluate

@hemanttanwar hemanttanwar merged commit 8928a8d into Azure:master Oct 23, 2020
@hemanttanwar hemanttanwar deleted the binarydata-api-update branch October 23, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srnagar srnagar srnagar approved these changes

@JonathanGiles JonathanGiles JonathanGiles approved these changes

@anuchandy anuchandy Awaiting requested review from anuchandy

@jianghaolu jianghaolu Awaiting requested review from jianghaolu

@alzimmermsft alzimmermsft Awaiting requested review from alzimmermsft

Assignees

No one assigned

Labels

Azure.Core azure-core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hemanttanwar @JonathanGiles @alzimmermsft @srnagar