Skip to content
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

Breaking changes introduced in 1.15 #5610

Closed
andreaTP opened this issue Oct 16, 2024 · 7 comments · Fixed by #5632
Closed

Breaking changes introduced in 1.15 #5610

andreaTP opened this issue Oct 16, 2024 · 7 comments · Fixed by #5632
Assignees
Labels
Java type:bug A broken experience WIP
Milestone

Comments

@andreaTP
Copy link
Contributor

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Linux executable

Client library/SDK language

Java

Describe the bug

The code generated for the GitHub API started to break starting from the 1.15 release.
The error is detected, but going unnoticed on main: https://github.com/microsoft/kiota/actions/runs/11341313558/job/31539450974#step:14:197
The IT tests are not reporting the failure 😢

Expected behavior

The GitHub API should produce code that compiles in Java.

How to reproduce

This IT tests are one way, alternatively, I found the error with those steps:

git clone https://github.com/kiota-community/kiota-java-extra.git
cd kiota-java-extra
mvn clean install

everything goes smooth, but, when you update the default Kiota version here:

https://github.com/kiota-community/kiota-java-extra/blob/54f5cd7a3e0e3df9993fe9f109a797d45eebba0d/maven/plugin/src/main/java/io/kiota/maven/plugin/KiotaMojo.java#L72
to anything >= 1.15.0

and re-run a:

mvn clean install

it will fail.

Open API description file

https://raw.githubusercontent.com/github/rest-api-description/129d610a96aaa0eb10627a3a8078708997d4512b/descriptions/api.github.com/api.git.luolix.top.json

Kiota Version

1.15.0+

Latest Kiota version known to work for scenario above?(Not required)

1.14.0

Known Workarounds

No response

Configuration

No response

Debug output

Click to expand log ```
</details>


### Other information

_No response_
@andreaTP andreaTP added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels Oct 16, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Oct 16, 2024
@msgraph-bot msgraph-bot bot added the Java label Oct 16, 2024
@baywet
Copy link
Member

baywet commented Oct 17, 2024

Hi @andreaTP
Thank you for using kiota and for reaching out.

There are a lot of errors in this integration test! Thanks for pointing this out.

First question: are you sure the regression was introduced by 1.5.0 and not a later version? Looking at the diff there has not been a lot of changes for java which could explain the regression.

Then, I'm a bit puzzled by this error that shows all over the place.

Error:  /home/runner/work/kiota/kiota/it/java/gh/src/main/java/models/GpgKey.java:[15,54] cannot find symbol
  symbol: class Parsable

here is an abstract of the code for reference

package apisdk.models;

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 java.time.OffsetDateTime;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
/**
 * A unique encryption key
 */
@jakarta.annotation.Generated("com.microsoft.kiota")
public class GpgKey implements AdditionalDataHolder, Parsable {

The Parsable symbol is rightfully imported, and if it couldn't find the dependency, it should also complain about AdditionalDataHolder (unless it reads from right to left and fails first?)

Just for sanity I put together #5624 to see whether it could be an issue with that older version of the packages and whether we can reduce the errors being reported so we can push the investigation further with other patterns.

Let us know if you have any additional comments or questions.

@baywet baywet self-assigned this Oct 17, 2024
@baywet baywet removed the status:waiting-for-triage An issue that is yet to be reviewed or assigned label Oct 17, 2024
@baywet baywet moved this from Needs Triage 🔍 to In Progress 🚧 in Kiota Oct 17, 2024
@baywet baywet added this to the Kiota v1.20 milestone Oct 17, 2024
@andreaTP
Copy link
Contributor Author

Hi @baywet , thanks for getting back the diff has been introduced in Kiota 1.15.0 as far as I understand, in the repro you can try yourself if I got anything configured wrong.

I think we should also get back the IT tests as they are not safeguarding us atm.

@andreaTP
Copy link
Contributor Author

I have investigated a little this issue, and seems like the first error we need to fix is the generation of this class:

package apisdk.appmanifests.item.conversions

public class Integration extends Integration implements Parsable {

which is not allowed Cyclic dependency.

@baywet
Copy link
Member

baywet commented Oct 18, 2024

ok so after looking more into it, all the parsable errors can be ignored. They are a side effect of the circular reference for the integration model.

This is caused by this operation /app-manifests/{code}/conversions#POST which has an inline model defined for the response, which inherits the reusable model "integration". That inline model gets named "integration" (hence the cyclic reference) when it should be named "ConversionsPostResponseBody" or something like that.

Looking at the diff between the versions, this is when I revamped the inherited models projection to capture a bunch of missing cases. I most likely introduced an uncaught regression in the naming for inline models here. What this means is this issue is not limited to Java. #4723

(looking into this further)

@andreaTP
Copy link
Contributor Author

They are a side effect of the circular reference for the integration model.

I'm a bit confused, not sure I follow the explanation, would you mind re-phrasing(at the end of digging this down is perfectly fine).

@baywet
Copy link
Member

baywet commented Oct 18, 2024

once I solved the circular reference, all the errors for not finding the reference to Parsable went away with no additional change.

@baywet
Copy link
Member

baywet commented Oct 18, 2024

I was able to identify and fix the issue on your PR #5632. I also identified and fixed a few other edge cases.
I took this opportunity to clean up some outdated suppressions and I'll follow up momentarily with my thoughts on #5631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants