Skip to content

fix: use the proper qualified struct tag as column cache key#14

Merged
hperl merged 2 commits intomainfrom
fix-column-cache-key
Dec 19, 2025
Merged

fix: use the proper qualified struct tag as column cache key#14
hperl merged 2 commits intomainfrom
fix-column-cache-key

Conversation

@hperl
Copy link

@hperl hperl commented Dec 19, 2025

No description provided.

Copilot AI review requested due to automatic review settings December 19, 2025 11:32
@hperl hperl requested a review from a team as a code owner December 19, 2025 11:32
@hperl hperl requested a review from aeneasr December 19, 2025 11:32
@hperl hperl self-assigned this Dec 19, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a cache collision issue in the column cache by changing the cache key from using just the table name to using a fully qualified type name (package path + type name). This prevents different struct types that map to the same database table from incorrectly sharing cached column definitions.

Key Changes

  • Added a new cacheKey() function that generates cache keys based on the fully qualified type name (PkgPath + Name)
  • Updated buildColumns() to use the new type-based cache key instead of the table name
  • Minor formatting changes to commented-out code for consistency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hperl hperl force-pushed the fix-column-cache-key branch from c4b18b2 to f4c800e Compare December 19, 2025 11:53
@hperl hperl requested a review from alnr December 19, 2025 12:16
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Note: depending on the use-case, you can also use .Select(colNames...). But this caching is really a pita...

@hperl
Copy link
Author

hperl commented Dec 19, 2025

Note: depending on the use-case, you can also use .Select(colNames...). But this caching is really a pita...

If there are additional column names, it won't cache anyways

if acl == 0 {

@zepatrik
Copy link
Member

If there are additional column names, it won't cache anyways

Yes that's what I wanted to say 😅

@hperl hperl requested a review from zepatrik December 19, 2025 12:44
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice, now we can have multiple structs for one table 😍
The question remains, is this now actually improving anything, or could we just remove the cache in general?

@hperl
Copy link
Author

hperl commented Dec 19, 2025

Nice, now we can have multiple structs for one table 😍 The question remains, is this now actually improving anything, or could we just remove the cache in general?

Well the cache is useful in general, because reflection is somewhat slow, so caching the columns per struct is still nice.

@hperl hperl merged commit 409c501 into main Dec 19, 2025
8 checks passed
@hperl hperl deleted the fix-column-cache-key branch December 19, 2025 14:25
@alnr
Copy link
Collaborator

alnr commented Dec 19, 2025

Nice, now we can have multiple structs for one table 😍 The question remains, is this now actually improving anything, or could we just remove the cache in general?

Well the cache is useful in general, because reflection is somewhat slow, so caching the columns per struct is still nice.

Right, but now we use reflection to compute the cache key...

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.

4 participants