Support azure-json in azure-core-management#38322
Conversation
alzimmermsft
requested review from
JonathanGiles,
anuchandy,
billwert,
lmolkova,
mssfang,
srnagar and
vcolin7
as code owners
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| if ("id".equals(fieldName)) { | ||
| ProxyResourceAccessHelper.setId(resource, reader.getString()); | ||
| } else if ("name".equals(fieldName)) { | ||
| ProxyResourceAccessHelper.setName(resource, reader.getString()); | ||
| } else if ("type".equals(fieldName)) { | ||
| ProxyResourceAccessHelper.setType(resource, reader.getString()); |
There was a problem hiding this comment.
We can do it here. But in SDK, seems won't able to do such in a subclass of Resource.
Maybe we do need to add a protected ctor.
There was a problem hiding this comment.
Yeah, I was thinking about adding that constructor. Though would we want to share that through all implementations, requiring these three values to be passed through the constructor.
|
@weidongxu-microsoft, it looks this PR may require a code generation update to resource manager libraries alongside with the changes made. |
Yes, some change would affect / get affected by codegen change. E.g. for I am OK with current code though. It does not add protected API so far, so we can delay the change till we design codegen. |
There was a problem hiding this comment.
LGTM
@XiaofeiCao
Is it possible we do a live run on some service (e.g. Compute, or a service with SystemData) with the unreleased core and core-management, before PR gets merged?
There was a problem hiding this comment.
I think these classes under polling namespace are for our internal use. It would be serialized to JSON so that it be persisted to the PollingContext.
| * @param value the value | ||
| * @return provisioning state or null if value cannot be parsed | ||
| */ | ||
| static String tryParseProvisioningState(String value, SerializerAdapter adapter) { |
There was a problem hiding this comment.
nit, Should we put a JsonSerializer interface on this API?
There was a problem hiding this comment.
We probably want some more test on the hybrid case (i.e. SubClass extends Resource, with Resource extends JsonSerializable, while SubClass does not).
Since the test requires dependency on unreleased core, I am fine you just add to your local, and put it after PR merge and both lib released.
alzimmermsft
deleted the
AzManagement_ImplementJsonSerializableInManagementCore
branch
4 participants
Description
Serializable model types in
azure-core-managementnow implementJsonSerializableto supportazure-json.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines