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

fix: correctly codegen paginators for types which require fully-qualified names #1249

Merged
merged 4 commits into from
Feb 27, 2025

Conversation

ianbotsf
Copy link
Contributor

Issue #

(none)

Description of changes

The codegen for paginators does not correctly handle the case of item types whose names collide with other types. Specifically, the function signature for the items paginator is formed without taking into account existing imports. This was discovered in a test model in which the pagination items were of type Flow which caused a paginator like this:

public fun Flow<FooResponse>.flows(): Flow<Flow> = ...

This PR improves the logic which forms item descriptors in paginator generators and will fully qualify type names which collide with existing imports, leading to this:

public fun Flow<FooResponse>.flows(): Flow<com.foo.Flow> = ...

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ianbotsf ianbotsf requested a review from a team as a code owner February 26, 2025 23:25

This comment has been minimized.

* @param initialRequest A [ListFlowsRequest] to start pagination
* @return A [kotlinx.coroutines.flow.Flow] that can collect [ListFlowsResponse]
*/
public fun TestClient.listFlowsPaginated(initialRequest: ListFlowsRequest = ListFlowsRequest { }): kotlinx.coroutines.flow.Flow<ListFlowsResponse> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: These will only be fully qualified kotlinx.coroutines.flow.Flow when there's an import collision, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, although typically kotlinx.coroutines.flow.Flow will be referenced first in our codegen and so a conflicting symbol like com.foo.Flow would actually be the one which gets the fully-qualified name in code. This test case shows it the other way around because the very first paginator is the one which has the collision and the model shape is referenced before kotlinx.coroutines.flow.Flow.

@@ -263,6 +263,130 @@ class PaginatorGeneratorTest {
actual.shouldContainOnlyOnceWithDiff(expectedImports)
}

@Test
fun testRenderPaginatorWithItemRequiringFqName() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Fq -> FullyQualified

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just Full instead?

@Test
fun testRenderPaginatorWithItemRequiringFullName() {

This comment has been minimized.

This comment has been minimized.

Copy link

Affected Artifacts

No artifacts changed size

@ianbotsf ianbotsf merged commit 72155f7 into main Feb 27, 2025
16 checks passed
@ianbotsf ianbotsf deleted the fix-paginators-with-naming-conflicts branch February 27, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants