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(java): Fix usage of cursor pagination causing endpoint return type to be void #5708

Merged
merged 24 commits into from
Jan 29, 2025

Conversation

ajgateno
Copy link
Member

This PR addresses a use case where a complex return type will make cursor paginated endpoints return void. PR description update pending more investigation.

@ajgateno ajgateno marked this pull request as ready for review January 28, 2025 22:28
@ajgateno ajgateno requested a review from dsinghvi as a code owner January 28, 2025 22:28
Copy link
Member

@amckinney amckinney left a comment

Choose a reason for hiding this comment

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

Nice! Left a few comments but this otherwise looks good to me as soon as the java-sdk snapshots are resolved (re: our discussion offline).

return reference.visit(new UnboxedTypeNameGetter(generatorContext, reference));
}

public static List<EnrichedCursorPathSetter> getPathSetters(
Copy link
Member

Choose a reason for hiding this comment

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

This function is quite long, so it could probably benefit from a variety of helper functions scoped to individual control flow statements.

It's fine to leave this as-is for now, but potentially something for us to revisit when we include those IR improvements to simplify this whole process.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make a linear ticket and put it on backlog

@@ -125,3 +127,4 @@ allowedFailures:
- unions
- streaming-parameter
- literal
- java-pagination-deep-cursor-path
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we still need this test definition alongside the testdata already included in the pagination testdata?

I think we have a lot of nested cursor pagination examples there, but feel free to leave this as-is if I'm wrong. If not here, wventually we'll want to consolidate the endpoints here in the pagination test definition so everyone can benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather leave it as is until the java one deals with these cases

Copy link
Member

@amckinney amckinney left a comment

Choose a reason for hiding this comment

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

The new ZeroValueUtils.java looks great 👏

@ajgateno ajgateno merged commit 8b2b96e into main Jan 29, 2025
51 of 53 checks passed
@ajgateno ajgateno deleted the alberto/add-pagination-test-case branch January 29, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants