Skip to content

Conversation

@kazimuth
Copy link
Contributor

@kazimuth kazimuth commented Mar 28, 2025

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
@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

CLA assistant check
All committers have signed the CLA.

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

@jdetter jdetter requested a review from rekhoff August 18, 2025 17:09
Signed-off-by: rekhoff <r.ekhoff@clockworklabs.io>
@rekhoff
Copy link
Contributor

rekhoff commented Aug 19, 2025

Attempting to merge master with this PR ran into build issues, mostly due to conflicts with #2725 .
I am working to resolve the conflicts and get another build going. I have an initial version ready that I need to test locally before pushing it to this PR.

@rekhoff rekhoff self-assigned this Aug 25, 2025
@rekhoff
Copy link
Contributor

rekhoff commented Aug 25, 2025

After additional fixes, all tests pass. I have tested the most recent changes against BitCraft and other minor tests, without issue.
I am making myself as an Assignee, due to the additional work. With that in mind, @jdetter would you mind reviewing/approving the changes?

@rekhoff
Copy link
Contributor

rekhoff commented Aug 26, 2025

Tested this change against the "fixture" test and adding the fields to a quickstart-chat project. Both succeeded (after a minor complication error that is now patched)

Copy link
Collaborator

@jdetter jdetter left a comment

Choose a reason for hiding this comment

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

This looks fine to me, me and Ryan have gone over these changes together + he has added a test to make sure the fields are working correctly. Thanks Ryan 👍

@rekhoff rekhoff added this pull request to the merge queue Aug 26, 2025
Merged via the queue into master with commit 0d2a45b Aug 26, 2025
25 checks passed
@rekhoff rekhoff deleted the jgilles/fix-read branch August 26, 2025 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot define table with column named "read"

7 participants