Skip to content

Fix fields named 'read' #2525

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix fields named 'read' #2525

wants to merge 1 commit into from

Conversation

kazimuth
Copy link
Contributor

@kazimuth kazimuth commented Mar 28, 2025 •
edited
Loading

Description of Changes

Fixes https://github.com/orgs/clockworklabs/projects/22?pane=issue&itemId=102392974&issue=clockworklabs%7Ccom.clockworklabs.spacetimedbsdk%7C276

by renaming internal static serializer fields so that they do not overlap with user-provided names.

Note, however, that some field names still will not work: ReadFields, WriteFields, Equals, and GetHashCode.
This would require a separate fix since the error would happen in a different place. In this case we would need to change the name of the generated member to something like ReadFields_ or Equals_, which I'm not sure is a good idea.

API and ABI breaking changes

N/A

Expected complexity level and risk

0

Testing

SDK tests and dotnet-verify tests are passing.

@kazimuth kazimuth self-assigned this Mar 28, 2025
@joshua-spacetime joshua-spacetime linked an issue Mar 28, 2025 that may be closed by this pull request
Cannot define table with column named "read" #2527
Open
@joshua-spacetime
Copy link
Collaborator

Note, however, that some field names still will not work: ReadFields, WriteFields, Equals, and GetHashCode.
This would require a separate fix since the error would happen in a different place. In this case we would need to change the name of the generated member to something like ReadFields_ or Equals_, which I'm not sure is a good idea.

Makes sense. What do you think @jdetter?

@bfops bfops added the release-any To be landed in any release window label Mar 31, 2025
@jdetter
Copy link
Collaborator

jdetter commented Mar 31, 2025

I don't super care about Equals or HashCode because I don't think we want users overriding these anyway. I think we should fix the cases for ReadFields and WriteFields because I could easily see someone creating a table that has a column with those names. (e.g. if you made a permissions table or something)

@kazimuth @joshua-spacetime does that sound good?

@jdetter
Copy link
Collaborator

jdetter commented Apr 12, 2025

@kazimuth Does this actually not have any API/ABI breaking changes? This doesn't require a C# SDK change?

@kazimuth kazimuth force-pushed the jgilles/fix-read branch from 92eeb08 to 10bbb80 Compare May 2, 2025 15:12
@CLAassistant
Copy link

CLAassistant commented May 3, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@kazimuth
Fix fields named 'read'
d53272f 
Add header comment

CSharpier

More comments
@kazimuth kazimuth force-pushed the jgilles/fix-read branch from 10bbb80 to d53272f Compare May 9, 2025 18:23
@kazimuth
Copy link
Contributor Author

Needs to be rebased once #2710 is merged, that's higher priority

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshua-spacetime joshua-spacetime Awaiting requested review from joshua-spacetime

@jdetter jdetter Awaiting requested review from jdetter

At least 1 approving review is required to merge this pull request.

Assignees

@kazimuth kazimuth

Labels
release-any To be landed in any release window
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot define table with column named "read"
5 participants
@kazimuth @joshua-spacetime @jdetter @CLAassistant @bfops