Add policy evaluation details related classes

Azure / autorest-clientruntime-for-java

The runtime libraries for AutoRest generated Java clients.

MIT License

20 stars 58 forks source link

Add policy evaluation details related classes #703

Closed lacikaaa closed 3 years ago

lacikaaa commented 3 years ago

PolicyViolationErrorInfo is missing @JsonIgnoreProperties(ignoreUnknown = true) annotation and ObjectMapper wasn't configured for ignoring unknown fields. This caused an exception when Azure returned evaluationDetails in it's response, making impossible to handle the error gracefully.

In this commit:

Configured ObjectMapper with DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES to avoid issues when a new field is introduced on Azure side
created EvaluationDetails and subsequent classes to parse the response
added toString implementation for related and newly created classes for easier logging
added unit test to verify the parsing

ghost commented 3 years ago

All CLA requirements met.

lacikaaa commented 3 years ago

@weidongxu-microsoft could you take a look? thanks

weidongxu-microsoft commented 3 years ago

Add @jianghaolu for review.

I am not familiar with this part. But I think fail on known properties should be already turned off? https://github.com/Azure/autorest-clientruntime-for-java/blob/master/client-runtime/src/main/java/com/microsoft/rest/serializer/JacksonAdapter.java#L151

OK maybe the ObjectMapper in CloudErrorDeserializer is on another configure. Do we want to turn FAIL_ON_UNKNOWN_PROPERTIES off there as well?

lacikaaa commented 3 years ago

OK maybe the ObjectMapper in CloudErrorDeserializer is on another configure. Do we want to turn FAIL_ON_UNKNOWN_PROPERTIES off there as well?

Shall I make this change? I'm happy to do it if it fits more to the current solution.

weidongxu-microsoft commented 3 years ago

OK maybe the ObjectMapper in CloudErrorDeserializer is on another configure. Do we want to turn FAIL_ON_UNKNOWN_PROPERTIES off there as well?

Shall I make this change? I'm happy to do it if it fits more to the current solution.

Let us wait a bit for Jianghao's input.

weidongxu-microsoft commented 3 years ago

@lacikaaa

BTW, which lib does you see this problem?

Asking because we had new generation SDK, but it is a bit more simplistic on policy violation error details. Considering whether we should improve it. https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/core/azure-core-management/src/main/java/com/azure/core/management/exception/AdditionalInfo.java

I've pinged Jianghao.

lacikaaa commented 3 years ago

we are using multiple which depend on this:

I think it would be a bigger effort on our side to switch to the new SDK even if it already supports everything we need.

thanks for the review!

weidongxu-microsoft commented 3 years ago

@lacikaaa

Sure, got it. Not asking you to migrate. Just want to make sure what is supported in current SDK will be supported in new SDK. So when people really need to migrate, they will find it easier.

lacikaaa commented 3 years ago

How do you feel about adding toString implementation to these classes? It would make logging easier, but I haven't seen it in the existing ones.

We have seen these policy validations a few times. I think it's pretty helpful if the reason can be extracted.

jianghaolu commented 3 years ago

It's missing here: https://github.com/Azure/autorest-clientruntime-for-java/blob/master/azure-client-runtime/src/main/java/com/microsoft/azure/PolicyViolation.java#L37. The best fix is to add

.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)

lacikaaa commented 3 years ago

@jianghaolu I have added the config and removed the annotations. also added toString implementation. I have updated the commit message to reflect the current state Could you take a look? thanks