Skip to content

Remove record by type #133

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.

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

Closed
wants to merge 3 commits into from
Closed

Remove record by type #133

wants to merge 3 commits into from

Conversation

BoD
Copy link
Collaborator

@BoD BoD commented Apr 23, 2025

Implements #113

@BoD BoD requested a review from martinbonnin as a code owner April 23, 2025 16:54
@@ -1,23 +1,27 @@
CREATE TABLE record (
key TEXT NOT NULL,
type TEXT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

How wasteful is it to call this typename? Because this is a typename, right?

Copy link
Collaborator Author

@BoD BoD Apr 23, 2025

Choose a reason for hiding this comment

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

Most of the time but actually I've added a fallback to the schema type for the case where __typename is not selected. But yeah typename may be a better name and I see no waste in that.

Copy link
Contributor

Choose a reason for hiding this comment

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

{
  node {
     id 
     ... on Product {
       price
     }
  }
}

This record will be stored with a typename of Node? That sounds dangerous. Might be worth throwing if typename is not queried?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This record will be stored with a typename of Node? That sounds dangerous.

Might be worth throwing if typename is not queried?

Interesting! I'm hesitant because this is only useful if you need to remove by type (a rare use case). Meanwhile requiring __typename on all selections is kind of a big change. On the other hand, __typename will already be present in most cases when using @typePolicy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make typename nullable in the SQL schema and throw (or ignore) in the removeByTypes() if the typename isn't known?

We should document what the use case is for removeByTypes(). My hunch is that if people want to remove a certain type of data for security or expiration reasons, they probably want typename all the time (and there is an option for that in the codegen)

@BoD
Copy link
Collaborator Author

BoD commented May 9, 2025

Closing for now as we don't have a clear for use-case for it. This can be re-opened later if needed.

@BoD BoD closed this May 9, 2025
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.

2 participants