Skip to content

fix: always return number of workspaces #12380

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

Closed
wants to merge 8 commits into from
Closed

fix: always return number of workspaces #12380

wants to merge 8 commits into from

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Mar 1, 2024

Related: #11795

WIP

@mtojek mtojek self-assigned this Mar 1, 2024
Comment on lines 306 to 307
LEFT JOIN
filtered_workspaces_order fwo
Copy link
Member Author

@mtojek mtojek Mar 1, 2024

Choose a reason for hiding this comment

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

@johnstcn @mafredri

I've applied the solution we chatted yesterday, and it works well on the SQL level, but when it comes to model structures, sqlc converted all properties of GetWorkspaceRows to nullables (see CI). The dumb solution would be to coalesce all properties with "zero" values, but wouldn't it be too dirty?

EDIT:

Is it the moment when I need to go through the information_schema?

EDIT2:

I think I will go with prepending an extra technical row with defaults. I don't want anyone accidentally messed up the workspace data.

Copy link
Member

@mafredri mafredri Mar 1, 2024

Choose a reason for hiding this comment

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

An ugly hack would be to ensure there is always one empty row of workspace and use inner join instead of left:

with filtered_workspaces_order as (
  select ...
  union all
  select '', '', ...
)

Maybe sqlc.embed could help too?

Copy link
Member

@johnstcn johnstcn Mar 1, 2024

Choose a reason for hiding this comment

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

Whenever I've had issues with sqlc 'incorrectly' inferring a nullable type, I've always worked around it by adding explicit type casts.

Edit: you also need to explicitly name the columns so they line up correctly with GetWorkspaceRow otherwise you get struct fields like FwoID and so on.

Something like:

	fwo.id :: uuid as id,
	fwo.created_at :: timestamptz as created_at,
	fwo.updated_at :: timestamptz as updated_at,
	fwo.owner_id :: uuid as owner_id,
	fwo.organization_id :: uuid as organization_id,
	fwo.template_id :: uuid as template_id,
	fwo.deleted :: boolean as deleted,
	fwo.name :: text as name,
	fwo.autostart_schedule as autostart_schedule, -- nullable so no typecast
	fwo.ttl as ttl, -- nullable so no typecast
	fwo.last_used_at :: timestamptz as last_used_at,
	fwo.dormant_at as dormant_at, -- nullable so no typecast
	fwo.deleting_at as deleting_at, -- nullable so no typecast
	fwo.automatic_updates :: automatic_updates as automatic_updates,
	fwo.favorite :: boolean as favorite,
	fwo.template_name :: text as template_name,
	fwo.template_version_id :: uuid as template_version_id,
	fwo.template_version_name :: text as template_version_name,
	fwo.username :: text as username,
	fwo.latest_build_completed_at :: timestamptz as latest_build_completed_at,
	latest_build_canceled_at :: timestamptz as latest_build_canceled_at,
	latest_build_error :: text as latest_build_error,
	latest_build_transition :: workspace_transition as latest_build_transition

Copy link
Member

Choose a reason for hiding this comment

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

Also note that you will need to manually update the scan order of the GetAuthorizedWorkspaces query when you make any changes to GetWorkspaces

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 might have sorted it out 👍 The query does not look great but works. I will work on tests and polishing. Thanks for guidance!

@@ -78,37 +78,6 @@ func TestUserDelete(t *testing.T) {
pty.ExpectMatch("coolin")
})

t.Run("UserID", func(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

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

for reviewers: duplicated function, probably bad merge

Copy link
Member Author

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Short update on what is happening:

I renamed GetWorkspaces to GetWorkspacesWithSummary, and wanted to keep GetWorkspaces and GetAuthorizedWorkspaces backward-compatible (no extra row), but now came to a point where I actually may need GetAuthorizedWorkspacesWithSummary?

I'd like to iterate one more time on this and make the extra row conditional (CASE).

@mtojek
Copy link
Member Author

mtojek commented Mar 4, 2024

I will open a clean PR 👍

@mtojek mtojek closed this Mar 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2024
@github-actions github-actions bot deleted the 11795-last-page branch August 31, 2024 00:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants