I'm not 100% sold on this. Today we're addingTemplateWithOwner, tomorrow do we addTemplateWithOwnerAndDependentWorkspaces? Where do we draw the line?

Given that the goal here is to support removing the ability for regular users to read all other user data, maybe we can allow all users to read a subset of other users' data, but disallow enumeration (e.g. allow GetPublicUserInfo but not ListAllUsers).

I intentionally only included the owner because this information is arguably always relevant and always ok to join. This PR replaces all instances ofTemplate in the codebase with this new one. If you cannot do that, than the newJOIN is not standard enough.

TemplateWithOwnerAndDependentWorkspaces is not as clear cut imo, and you would realize there are cases where you do not want the dependencies.

So the line would be just that. What is the base standard information to include in the "primitive" resource. If it comes to be we should include say the "Active template version name", then I think it could be argued to join that in too in the future.

Naming gets annoying though...TemplateWithOwnerAndVersionName sucks. MaybeExtendedTemplate,ExpandedTemplate, ... ?

I did try usingsqlc.embed and skipping sqlc and using sqlx and manual queries.

The downside tosqlc.embed was the new struct for each endpoint and the fixed method signatures. An idea I had was to make some interface like:

typeTemplateinterface {Template() database.Template}

Then if I join more tables, it just returns interfaces with more fields, but it's still aTemplate.

typeTemplateWithOwnerinterface {TemplateOwner()VisibleUser}

The nextJOIN might be:

typeTemplateWithDependenciesinterface {TemplateTemplateWithOwnerActiveVersion() database.TemplateVersion}

This would allow us to still write general functions for things likeConvertTemplate() and what not.

I would use sqlx to make these models and method signatures.

The downside to sqlc.embed was the new struct for each endpoint and the fixed method signatures.

Would it be reasonable to patchsqlc to support naming embeds, thus only creating a single named struct (all same-named embeds should have the exact same fields)?

Another idea, would it be reasonable to move the "private" user data into a separate table? This way we can assume querying the regular users table is always OK (sans listing all users). With the private data in a separate table, it requires explicit intent to include it. This may be a bad suggestion as my understanding of what we're trying to do/achieve here is a bit limited.

Would it be reasonable to patchsqlc to support naming embeds, thus only creating a single named struct (all same-named embeds should have the exact same fields)?

This would solve the new struct per query problem, but it would still leave the problem of copy pasting the SQL table being joined. The view prevents having to copy/paste this new "Templates" table on each query. Views cannot use thesqlc.embed functionality, so I lose the ability to decompose theTemplate from theTemplateWithUser easily.

SELECT     templates.*,users.usernameAS created_by_username,users.avatar_urlAS created_by_avatar_urlFROM    templatesLEFT JOIN    (%s)AS usersONtemplates.created_by=users.id

Another idea, would it be reasonable to move the "private" user data into a separate table? This way we can assume querying the regular users table is always OK (sans listing all users). With the private data in a separate table, it requires explicit intent to include it. This may be a bad suggestion as my understanding of what we're trying to do/achieve here is a bit limited.

This was my idea with theVisibleUsers view. I did not want to make it a new table, because then all User queries would need toJOIN with the new "Public fields" table. And usingJOINs is the problem, so trying not to decompose more resources. I think theVisibleUsers view is fair though because it should never change. As in, the public fields on the user are relatively static.

@johnstcn@mafredri One way to get around a lot of the type stuff is just to make this the newtemplate type. I could use thesqlc.yaml to renameTemplateWithUser ->Template.

All our code will just use this new view. There still is the issue of updating the view on new columns, and that is just a new chore we will have. I can even add a unit test to compare the fields onTemplate andTemplateTable or something and print an error message of what to do.

@johnstcn I know this doesn't solve the slippery slope of where to stop (TemplateWithDependencies etc)

There is the issue of solving the greaterJOIN problem. The greaterJOIN problemwill be more code changes and change how we write queries. This current PR issue is a bit different since I am effectively just adding "username" and "avatar_url" columns toTemplate,WorkspaceBuild, andTemplateVersion. Obviously I need to pull these dynamically.

@coadler also pointed out a concern for rebuilding the views. I think I can come up with a unit test that will error nicely. You would have to run CI to see said error though 😢

EDIT: Maybe in themake gen code I can run a checker that will print something 🤔

@Emyrk can we just maintain our own slight fork of sqlc? We depend on it so heavily that we shouldn't be limited by the most common use-cases imo.

I like the idea that@mafredri has with naming embeds...

@Emyrk can we just maintain our own slight fork of sqlc? We depend on it so heavily that we shouldn't be limited by the most common use-cases imo.

We would probably only need to fork the "plugin":https://github.com/sqlc-dev/sqlc-go

https://docs.sqlc.dev/en/latest/guides/plugins.html

There is 2 things we would need to support to get this "clean".

• Support reusing models for multiple queries (Something likeSupport for custom models sqlc-dev/sqlc#755 orStruct Overrides (proposal) sqlc-dev/sqlc#1176)
• Reusing of queries to essentially use "view"s without making an actual view. (something likeQuery templates sqlc-dev/sqlc#112)

I would be in favor of this fork idea. For this specific PR though I would prefer just to use theVIEW.

I think forking sqlc would be a very interesting idea though that opens up possibilities.

@matifali@johnstcn@kylecarbs

For this PR and the next typesWorkspaceBuild andTemplateVersion, I am making aVIEW toreplace the existing type. We will only use theVIEW.

If we want to change this in the future to support more joins or whatever, we can address it then. I do not want to fork SLQc, or use SQLx, or write custom queries, or ..etc.. right now.

JOIN support can come, and do not think this PR would prevent that. I say we do this simple thing now, and when we hit another 2 or 3 cases, we find a better way to generalize it.

Would be nice if we could patch the db interface to do the update/select in one place vs. everywhere (avoid cost of no returning*). But it's not clear to me how we'd do that.

We'd need some layer between the db and thedatabase.Store, which we currently don't have. So there isn't really a trivial way to do it unfortunately.

Btw, do API docs need updating? I see there was no change in documented/generated return types.

Should not be. The fields were always in the api, we just had to do aGetUserByID before.