Skip to content

IFC-2156: Remove proposed change comments created_at and created_by attribute and relationship#8362

Open
solababs wants to merge 3 commits intodevelopfrom
sb-12022026-remove-created-by-on-comments-ifc-2156
Open

IFC-2156: Remove proposed change comments created_at and created_by attribute and relationship#8362
solababs wants to merge 3 commits intodevelopfrom
sb-12022026-remove-created-by-on-comments-ifc-2156

Conversation

@solababs
Copy link
Contributor

@solababs solababs commented Feb 12, 2026

Why

CoreComment and CoreThread explicitly define created_by (relationship) and created_at (attribute) in their schemas, but these are redundant — every node already has created_at and created_by as node metadata. This adds unnecessary schema complexity and graph relationship overhead.

This PR removes these redundant fields so that comments and threads rely on the built-in node metadata

Closes IFC-2156

What changed

  • Schema definitions: Removed created_at attribute and created_by relationship from CoreComment and CoreThread generic schemas, along with the order_by=["created_at__value"] that depended on the removed attribute

  • Protocol definitions: Removed created_at and created_by from CoreComment and CoreThread in both protocols.py and tests/protocols.py

  • Graph migration (m060): Backfills created_by UUID from the old comment__account and thread__account relationships directly onto the node property, preserving existing data

  • Test fix: Removed created_at input from the CoreChangeThreadCreate mutation test since it's no longer an input field

How to review

Start with the schema removal in propose_change_comment.py, then the protocol updates.
The migration in m060_set_comment_thread_created_by_on_node.py.
The test for the migration validates both comment and thread backfill.

How to test

# Unit tests
uv run invoke backend.test-unit

# Migration test
uv run pytest backend/tests/component/core/migrations/graph/test_060_set_comment_thread_created_by_on_node.py -xvs

# Proposed change mutation test (verifies thread creation without created_at)
uv run pytest backend/tests/component/graphql/mutations/test_proposed_change.py -xvs

Impact & rollout

  • Backward compatibility:
    Migration m060 preserves existing created_by data by copying it from relationships to node properties. The created_at and created_by fields are no longer exposed as schema attributes/relationships in the GraphQL API — consumers should use node_metadata instead.

  • Performance:
    Slight improvement — fewer relationship traversals needed for comment/thread author lookups

  • Deployment notes:
    Migration m060 runs automatically on startup. Frontend will need a corresponding update to read created_by from node_metadata instead of from the comment/thread relationship.

Checklist

@solababs solababs requested a review from a team as a code owner February 12, 2026 09:15
@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Feb 12, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 12, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing sb-12022026-remove-created-by-on-comments-ifc-2156 (cab12a3) with develop (3cf38ce)

Open in CodSpeed

CALL (comment) {
MATCH (comment)-[r1:IS_RELATED]-(:Relationship {name: "comment__account"})-[r2:IS_RELATED]-(account:CoreGenericAccount)
RETURN account
ORDER BY r1.from DESC, r2.from DESC
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think either of these suggestions is required b/c this is a required cardinality-one relationship that should never be updated, but I haven't reviewed the schema, so not totally sure

  • if the relationship is capable of being updated, you'll need to use r1.from DESC, r1.status ASC, r.from DESC, r2.status ASC for ordering to account for relationships added and deleted at the same timestamp
  • if the relationship is optional, you'll need to account for checking if the edges are status="active" b/c there the latest relationship could be deleted

Copy link
Contributor

@ogenstad ogenstad left a comment

Choose a reason for hiding this comment

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

As this work also requires frontend changes, let's wait with this one until after the feature freeze or when we cut a release-1.8 branch this can be merged into develop for inclusion in 1.9 instead. So I don't have any objections to the code itself only that we should wait a few days.

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

Labels

group/backend Issue related to the backend (API Server, Git Agent)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants