-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[POC] A draft version for the search serializer to collect feedback #13218
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
Conversation
| * Deserializes a JSON object into a {@link Geometry}. | ||
| */ | ||
| final class GeometryDeserializer extends JsonDeserializer<Geometry> { | ||
| public final class GeometryDeserializer extends JsonDeserializer<Geometry> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for testing this, in the future when this work is merged into Azure Core it'll become package private again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I think it doesn't hurt to public the serializer before we move back Azure Core.
| exports com.azure.core.experimental.serializer; | ||
| exports com.azure.core.experimental.spatial; | ||
|
|
||
| opens com.azure.core.experimental.spatial to com.fasterxml.jackson.databind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't believe this needs to open to databind as no databinding will happen with the types in spatial. They all should be using streaming deserialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It indeed complaint about the PointGeometry did not open module to databind. Why spatial doesn't need to do reflection when serialize?
| * @return A new instance of {@link JacksonSearchSerializer}. | ||
| */ | ||
| public JacksonJsonSerializer build() { | ||
| public JacksonSearchSerializer build() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the Azure Core implementation renamed? Just prototyping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Caused by IDE refactoring
| /** | ||
| * Interface to be implemented by an azure-core plugin that wishes to provide a {@link SearchSerializer} implementation. | ||
| */ | ||
| public interface SearchSerializerProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we would want to stay away from the Search serializer modules having a service provider interface, I feel instead they should wrap the Azure Core variant otherwise it'll be confusing to know what serialization implementation to use.
Another option would be the Search variants implement and provide the Core interface instead of their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have posted in team channel. Search needs to have type as parameter otherwise we are not able to do map serialization.
I am ok to move to azure-core-experiment module API if it provides the corresponding API
| public abstract class SearchType<T> implements Comparable<SearchType<T>>{ | ||
| private final Type _type; | ||
|
|
||
| public SearchType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we add this class? If required we can change the interface in Core from taking a Class<T> to Type which would support both Class<T> and ParameterizedType which this seems to be attempting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By having SearchType, we can hide the ugly type reflection operation behind for user.
Map<String, Object> mapInstance = new HashMap<>;
Type type = ((ParameterizedType)mapInstance.getClass().getGenericSuperclass()).getActualTypeArguments()[0];
|
/azp run java - search - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.