-
Notifications
You must be signed in to change notification settings - Fork 134
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
Generated models and request builders #2163
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
107 changes: 107 additions & 0 deletions
107
.../java/com/microsoft/graph/generated/applications/item/restore/RestorePostRequestBody.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| package com.microsoft.graph.applications.item.restore; | ||
|
|
||
| import com.microsoft.kiota.serialization.AdditionalDataHolder; | ||
| import com.microsoft.kiota.serialization.Parsable; | ||
| import com.microsoft.kiota.serialization.ParseNode; | ||
| import com.microsoft.kiota.serialization.SerializationWriter; | ||
| import com.microsoft.kiota.store.BackedModel; | ||
| import com.microsoft.kiota.store.BackingStore; | ||
| import com.microsoft.kiota.store.BackingStoreFactorySingleton; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| @jakarta.annotation.Generated("com.microsoft.kiota") | ||
| public class RestorePostRequestBody implements AdditionalDataHolder, BackedModel, Parsable { | ||
| /** | ||
| * Stores model information. | ||
| */ | ||
| @jakarta.annotation.Nonnull | ||
| protected BackingStore backingStore; | ||
| /** | ||
| * Instantiates a new {@link RestorePostRequestBody} and sets the default values. | ||
| */ | ||
| public RestorePostRequestBody() { | ||
| this.backingStore = BackingStoreFactorySingleton.instance.createBackingStore(); | ||
| this.setAdditionalData(new HashMap<>()); | ||
| } | ||
| /** | ||
| * Creates a new instance of the appropriate class based on discriminator value | ||
| * @param parseNode The parse node to use to read the discriminator value and create the object | ||
| * @return a {@link RestorePostRequestBody} | ||
| */ | ||
| @jakarta.annotation.Nonnull | ||
| public static RestorePostRequestBody createFromDiscriminatorValue(@jakarta.annotation.Nonnull final ParseNode parseNode) { | ||
| Objects.requireNonNull(parseNode); | ||
| return new RestorePostRequestBody(); | ||
| } | ||
| /** | ||
| * Gets the AdditionalData property value. Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well. | ||
| * @return a {@link Map<String, Object>} | ||
| */ | ||
| @jakarta.annotation.Nonnull | ||
| public Map<String, Object> getAdditionalData() { | ||
| Map<String, Object> value = this.backingStore.get("additionalData"); | ||
| if(value == null) { | ||
| value = new HashMap<>(); | ||
| this.setAdditionalData(value); | ||
| } | ||
| return value; | ||
| } | ||
| /** | ||
| * Gets the autoReconcileProxyConflict property value. The autoReconcileProxyConflict property | ||
| * @return a {@link Boolean} | ||
| */ | ||
| @jakarta.annotation.Nullable | ||
| public Boolean getAutoReconcileProxyConflict() { | ||
| return this.backingStore.get("autoReconcileProxyConflict"); | ||
| } | ||
| /** | ||
| * Gets the backingStore property value. Stores model information. | ||
| * @return a {@link BackingStore} | ||
| */ | ||
| @jakarta.annotation.Nonnull | ||
| public BackingStore getBackingStore() { | ||
| return this.backingStore; | ||
| } | ||
| /** | ||
| * The deserialization information for the current model | ||
| * @return a {@link Map<String, java.util.function.Consumer<ParseNode>>} | ||
| */ | ||
| @jakarta.annotation.Nonnull | ||
| public Map<String, java.util.function.Consumer<ParseNode>> getFieldDeserializers() { | ||
| final HashMap<String, java.util.function.Consumer<ParseNode>> deserializerMap = new HashMap<String, java.util.function.Consumer<ParseNode>>(1); | ||
| deserializerMap.put("autoReconcileProxyConflict", (n) -> { this.setAutoReconcileProxyConflict(n.getBooleanValue()); }); | ||
| return deserializerMap; | ||
| } | ||
| /** | ||
| * Serializes information the current object | ||
| * @param writer Serialization writer to use to serialize this model | ||
| */ | ||
| public void serialize(@jakarta.annotation.Nonnull final SerializationWriter writer) { | ||
| Objects.requireNonNull(writer); | ||
| writer.writeBooleanValue("autoReconcileProxyConflict", this.getAutoReconcileProxyConflict()); | ||
| writer.writeAdditionalData(this.getAdditionalData()); | ||
| } | ||
| /** | ||
| * Sets the AdditionalData property value. Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well. | ||
| * @param value Value to set for the AdditionalData property. | ||
| */ | ||
| public void setAdditionalData(@jakarta.annotation.Nullable final Map<String, Object> value) { | ||
| this.backingStore.set("additionalData", value); | ||
| } | ||
| /** | ||
| * Sets the autoReconcileProxyConflict property value. The autoReconcileProxyConflict property | ||
| * @param value Value to set for the autoReconcileProxyConflict property. | ||
| */ | ||
| public void setAutoReconcileProxyConflict(@jakarta.annotation.Nullable final Boolean value) { | ||
| this.backingStore.set("autoReconcileProxyConflict", value); | ||
| } | ||
| /** | ||
| * Sets the backingStore property value. Stores model information. | ||
| * @param value Value to set for the backingStore property. | ||
| */ | ||
| public void setBackingStore(@jakarta.annotation.Nonnull final BackingStore value) { | ||
| Objects.requireNonNull(value); | ||
| this.backingStore = value; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Flagged breaking changes seem to be a bug fix to allow posting a body.
cc; @baywet @andrueastman
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.
first question: how did you spot it? was the new workflow any help?
then, from the docs, it seems the service supports BOTH sending or not sending a body.
our XSLT doesn't have any modification for this, and the parameter is optional
The workload added the new optional parameter to existing actions. (default for nullable is true when not specified)
This is what OAS we have for this operation since the change:
And we're missing information here from a strict OpenAPI perspective.
It should instead be
'/directory/deletedItems/{directoryObject-id}/restore': post: requestBody: content: application/json: schema: type: object properties: autoReconcileProxyConflict: type: boolean default: false nullable: true required: true + */*: + schema: + type: nullBut even if we fixed that in yoko, kiota would probably ignore this second entry as is. Even if we fixed that, we'd then need to add overloads in the code dom, and make sure they translate well in every language. (e.g. in typescript, it wouldn't be an overload, but an optional parameter instead).
These are a big set of changes that need to be coordinated across the board. And I'd like to get @darrelmiller to validate my analysis here first.
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.
type: null is not allowed in OpenAPI 3.0, so that isn't an option. https://spec.openapis.org/oas/v3.0.3.html#data-types
Maybe I am missing something here, but isn't the solution to set requestBody.required = false when all of the properties of the request payload are optional?
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 now kiota is not considering this field, but we could vary based on that, and if the body is NOT required, in the context of Java, project an overload without the body parameter.
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.
Yes. Related to comment that is also at microsoftgraph/msgraph-sdk-dotnet#2671 (comment)
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.
Created this for now microsoft/OpenAPI.NET.OData#582
Thinking about this a bit more though, if a required parameter is added to this specific action for example, wouldn't we still have a similar problem? The request body would now become required making the optional overloads "dissappear".
From the generation perspective, would it make sense to always generate a requestBody parameter and have the marshalling not send anything in the scenario with an empty payload/object?
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.
Wouldn't adding a required property to a previously optional body be a breaking change on the API side? And if an API is happy with that, then the SDKs should reflect the breaking change?
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.
Exactly, adding a required parameter to an action is a breaking change at the API level. This is why I didn't consider this scenario.
We'd still have a potential breaking change though in the current scenario if we use optional parameters in the language. I'm not sure there's a solution to that besides not using optimal parameters which comes with duplication etc...
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.
from the meeting today: we could/should patch the metadata in the XSLT in the meanwhile so we don't block all the releases. @Ndiritu can you create in an issue in the metadata repo so the work is prioritized please?
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.
Created: microsoftgraph/msgraph-metadata#694