8000 fix: list templates returns non-deprecated templates by default by ssncferreira · Pull Request #17747 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

fix: list templates returns non-deprecated templates by default #17747

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 13, 2025
Merged

fix: list templates returns non-deprecated templates by default #17747

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
79313cc
fix: list templates returns non-deprecated templates by default
ssncferreira May 9, 2025
db52150
docs: update method docstrings for fetching templates and organizatio…
ssncferreira May 12, 2025
3e5e40d
fix: comment in enterprise/coderd/templates_test.go
ssncferreira May 12, 2025
fa8f999
refactor: add comment to coderd/database/dbmem/dbmem.go
ssncferreira May 12, 2025
b344266
fix: move templates tests from enterprise/coderd/templates_test.go to…
ssncferreira May 12, 2025
12985c5
Merge remote-tracking branch 'origin/main' into ssncferreira/fix-depr…
ssncferreira May 12, 2025
06acc27
refactor(dbmem): improve deprecation filtering logic with a helper fu…
ssncferreira May 13, 2025
c88d4d2
Merge remote-tracking branch 'origin/main' into ssncferreira/fix-depr…
ssncferreira May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: add comment to coderd/database/dbmem/dbmem.go
  • Loading branch information
@ssncferreira
ssncferreira committed May 12, 2025
commit fa8f99992887e89df725e7696c322350887a6e83
1 change: 1 addition & 0 deletions coderd/database/dbmem/dbmem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13021,6 +13021,7 @@ func (q *FakeQuerier) GetAuthorizedTemplates(ctx context.Context, arg database.G
if arg.ExactName != "" && !strings.EqualFold(template.Name, arg.ExactName) {
continue
}
// Check if the search query filter 'Deprecated' status matches the template's 'Deprecated' status
if arg.Deprecated.Valid && arg.Deprecated.Bool != (template.Deprecated != "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment here might be useful as well. Initially I wanted to suggest renaming arg.Deprecated but I imagine that's a bigger change to roll out here. It's just not very nice to read this as something like

if arg.Deprecated.Valid && arg.IgnoreDeprecated && template.IsDeprecated

potentially the template struct could be refactored to have IsDeprecated and DeprecatedMessage as opposed to just having Deprecated message whose value being "some string" means the template is deprecated. Alternatively, the template could have a function IsDeprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the current expression with arg.Deprecated.Valid && arg.IgnoreDeprecated && template.Deprecated.Valid isn't the most readable.

Just for context: this method filters templates based on the arg param in the in-memory database used for testing. This logic ensures that only templates matching the requested deprecated state are returned.

The Deprecated field is of type sql.NullBool, which is represented as:

type NullBool struct {
	Bool  bool
	Valid bool // Valid is true if Bool is not NULL
}

So we need to check Valid to avoid misinterpreting a NULL as false.

The template is of type database.Template, generated by sqlc.
I could add a method IsDeprecated to the Template struct to clean this up, but since that type is generated by sqlc, don't think it would be a good idea to add this type of custom logic directly at the DB layer. An alternative might be to define a wrapper around the generated type or introduce a view model in the service layer that handles this logic more cleanly but that may add more complexity than it solves, especially for a test-specific filter.

Copy link
Member

Choose a reason for hiding this comment

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

For readability I'd suggest a comment at most, or some light variable extraction. dbmem is only used for testing and it is planned to retire it at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cstyan added a comment in fa8f999

Copy link
Member

Choose a reason for hiding this comment

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

Reading this change a little more closely, doesn't this invert the query logic?

Comparing with the SQL:

	-- Filter by deprecated
	AND CASE
		WHEN sqlc.narg('deprecated') :: boolean IS NOT NULL THEN
			CASE
				WHEN sqlc.narg('deprecated') :: boolean THEN
					deprecated != ''
				ELSE
					deprecated = ''
			END
		ELSE true
	END

Aside: I've found it useful to comment the Go code in dbmem with the relevant parts of the corresponding SQL query.

Copy link
Contributor Author
@ssncferreira ssncferreira May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, in dbmem we are doing an inverse filtering to exclude non-matching templates.

In the SQL, we filter as follows:

  • If deprecated is set to true, we only include templates where deprecated != '', i.e., there is a deprecation message.
  • If deprecated is set to false, we only include templates where deprecated = '', i.e., there is no deprecation message.
  • If deprecated is not set (NULL), we include all templates (ELSE true clause).

In the in-memory logic we are doing an inverse filtering, the continue is used to exclude non-matching templates:

if arg.Deprecated.Valid && arg.Deprecated.Bool != (template.Deprecated != "") {
  continue
}

We skip templates when the Deprecated filter is set and the template's deprecation status does not match the Deprecated value. This mirrors the behavior of the SQL CASE logic.

I think what’s confusing here is the double negation. I’ll improve the comment and maybe add a simple isDeprecated function for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduced a helper function isDeprecated and improved the comment by adding the corresponding SQL logic for better readability: 06acc27
@cstyan @johnstcn let me know if this makes it clearer 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I like it

continue
}
Expand Down
Loading
0