-
Notifications
You must be signed in to change notification settings - Fork 203
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
BackingStore fixes for Java #3643
Conversation
6e194f4
to
7d578b5
Compare
@ramsessanchez I notice in the generation at microsoftgraph/msgraph-sdk-java#1600 there are still failures for the nullability annotations for |
CHANGELOG.md
Outdated
@@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Changed | |||
|
|||
- Fixed a bug where query parameter enum reusable types would be trimmed. [#3637](https://github.com/microsoft/kiota/issues/3637) | |||
- Fixes bug where import statements for additionalDataHolder and enumSet are missing when BackingStore is enabled in java. [#3643](https://github.com/microsoft/kiota/pull/3643) |
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.
please move this to the change section under the unreleased version
@@ -1502,7 +1502,6 @@ public void WritesSerializerBody() | |||
Assert.Contains("writeCollectionOfPrimitiveValues", result); | |||
Assert.Contains("writeCollectionOfObjectValues", result); | |||
Assert.Contains("writeEnumValue", result); | |||
Assert.Contains("writeAdditionalData(this.getAdditionalData());", result); |
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.
can you provide more context on why this is being removed? it feels like we should keep it
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.
Via the change we add the additionalData serialization through the accessedProperty not on the class properties themselves, the test covers the case where the property is added, however that will not lead to the serialization since the getter is not present. The correct test would be to add the additionalData getter and show that the serializer will in fact generate this. I think the serializer method should rely on the accessedProperties of the available getters as this will ensure that the getSomeProperty() method will be available. For context this is the change:
var additionalDataProperty = parentClass.GetPropertyOfKind(CodePropertyKind.AdditionalData);
if (additionalDataProperty != null)
writer.WriteLine($"writer.writeAdditionalData(this.get{additionalDataProperty.Name.ToFirstCharacterUpperCase()}());");
I considered the following where we actually have both conditions, let me know your thoughts
if (parentClass.Methods.Any(x => x.AccessedProperty?.IsOfKind(CodePropertyKind.AdditionalData) ?? false) ||
parentClass.Properties.Any(x => x?.IsOfKind(CodePropertyKind.AdditionalData) ?? false))
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.
thanks for clarifying. Please make that change instead (getter + accessed property) and restore the line in the unit test
Yes, I noticed that as well, I believe this is another side effect of the backingStore being enabled as these parameters are not present in generation when backing store is disabled. |
Pausing on this work for now to focus on the async to sync revert work. |
Kudos, SonarCloud Quality Gate passed! |
Superseded by #3702 |
-Import statements for additionalDataHolder and enumSet were previously based on class properties, when backing store is enabled these properties are not generated so the import statements are missing.
Instead, the import statements are now based on the AccessedProperty of the methods present in the class.