[Tables] Fix Deserialization Issues within ListEntities #40392
Conversation
|
Need to add mirror tests to the asynchronous client. |
|
API change check API changes are not detected in this pull request. |
|
Need to add a follow-up to transition disabled tests to live-only. |
| @@ -50,7 +50,7 @@ public static TableEntity createEntity(Map<String, Object> properties) { | |||
| } | |||
| } | |||
|
|
|||
| return creator.get().setProperties(properties); | |||
| return creator.get().setProperties(TableUtils.adjustOdataPropertyMap(properties)); | |||
There was a problem hiding this comment.
L42 - should you be checking properties for null at the method start
There was a problem hiding this comment.
@joshfree That is not necessary because adjustOdataPropertyMap makes use of computeIfPresent from Java.util.Map which will ignore null properties.
| byte[] bytes = java.util.Base64.getDecoder().decode(((String) adjustedMap.get(key)).getBytes()); | ||
| return bytes; | ||
| } | ||
| } else if ("Edm.DateTime".equals(odataType)) { |
There was a problem hiding this comment.
These string literals are in at least two places in the code now.
Can you make these protected static final and use them here and in the EntityDataModelType where they're also used?
| } | ||
|
|
||
|
|
||
| @Disabled("This test is disabled because it is not supported using MI.") |
There was a problem hiding this comment.
@joshfree My plan was to mark them Live-Only in a follow-up PR but I can just do that now. I was having issues with merging from main earlier but my rebase went cleanly so it should be fine now.
jairmyree
force-pushed
the
tables/bug/OffsetDateTime-as-String
branch
from
7173273 to
23e2b12
Compare
Making SAS token tests live only
Making SAS token tests live only
jairmyree
force-pushed
the
tables/bug/OffsetDateTime-as-String
branch
from
e3c5698 to
6570051
Compare
| @@ -17,7 +17,7 @@ | |||
| * The properties for the table entity query response. | |||
| */ | |||
| @Fluent | |||
| public final class TableEntityQueryResponse implements JsonSerializable<TableEntityQueryResponse> { | |||
| public final class TableEntityQueryResponse extends TablesJacksonSerializer implements JsonSerializable<TableEntityQueryResponse> { | |||
There was a problem hiding this comment.
From an API perspective, I think it would be better for TableEntityQueryResponseto have a constant property of aTablesJacksonSerializerrather than extending it. Every time an instance ofTableEntityQueryResponseis instantiated it will create a new instance ofJacksonAdapteras super types are initialized on creation. This will have a large overhead as this will create newObjectMapper`'s each time, which is expensive.
| @@ -122,7 +122,7 @@ private static boolean shouldGetEntityFieldsAsMap(Type type) { | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| private static <U> U deserializeTableEntityQueryResponse(JsonReader jsonReader) throws IOException { | |||
| protected static <U> U deserializeTableEntityQueryResponse(JsonReader jsonReader) throws IOException { | |||
There was a problem hiding this comment.
I'd just make this public, protected really doesn't get us anything.
| /** | ||
| * Table service data types. | ||
| */ | ||
| public abstract class TableServiceDataTypes { |
There was a problem hiding this comment.
I don't see this type being used anywhere other than in EntityDataModelType, does this class need to exist to hold these constants or can they be constants in EntityDataModelType instead?
|
@jairmyree Do you when this will be available in a release on maven central? I just ran into this issue also that was causing code to break. |
|
@rwmajor2 azure-data-tables 12.4.2 was just released today. Outside of the time it takes to propagate through maven central, it should be available immediately. |
After the azure-json migration, there was a Tables bug arising where data types were not being deserialized appropriately.
This PR fixes the deserialization issues and adds additional testing to ensure that
getEntityandlistEntitiesmaintain parity on expected results.