Skip to content
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

Added a column disallow list in the content API posts serializer #20207

Merged
merged 7 commits into from
May 20, 2024

Conversation

9larsons
Copy link
Contributor

ref https://linear.app/tryghost/issue/CFR-29

  • Removed the mobiledoc and lexical columns from the posts input serializer, meaning they will no longer be queried for.

Get helpers are essentially a gateway to the Content API. We already strip out the mobiledoc and lexical fields in the output serializer/returned response, but this means we're passing the mobiledoc and lexical fields back from the db. This is pointless and these fields are substantial in size - by far the largest fields in the whole ghost db - leading to slowed performance.

I've updated the input serializer to strip out the lexical and mobiledoc columns so we stop doing a select * with every query.

ref https://linear.app/tryghost/issue/CFR-29
- Removed the mobiledoc and lexical columns from the posts input serializer, meaning they will no longer be queried for.

Get helpers are essentially a gateway to the Content API. We already strip out the mobiledoc and lexical fields in the output serializer/returned response, but this means we're passing the mobiledoc and lexical fields back from the db. This is pointless and these fields are substantial in size - by far the largest fields in the whole ghost db - leading to slowed performance.

I've updated the input serializer to strip out the lexical and mobiledoc columns so we stop doing a `select *` with every query.
@9larsons
Copy link
Contributor Author

I'm not sure of a better way to grab the column list. Knex appears to have something we may be able to use here.

@daniellockyer
Copy link
Member

I'm not sure of a better way to grab the column list.

IMO this is the best way - the schema file is a cheap way to lookup our DB state

Copy link
Contributor

@cmraible cmraible left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Feels like a big win 🎉

frame.options.columns = frame.options.columns.filter((column) => {
return !['mobiledoc', 'lexical'].includes(column);
});
} else if (frame.options.selectRaw) {
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 feel strongly about this but I wonder if we should strip these columns out if they're explicitly being asked for in selectRaw. I don't think we do this anywhere currently, so this is more of a maintenance concern than anything, but if we have some future use-case where we need to parse the lexical to generate some calculated field, I could see this causing me an hour or two of confusion when I can't get these fields returned no matter what I do.

Not a blocker at all, just a thought

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be worth running in to something like this and recognizing the performance concern of doing so. I'm split 50-50 on it and considered this earlier, just going to go with it as-is for now. I think/hope we should be moving to some sort of pattern to accomplish this for all of our queries - especially with the content API - and we can revisit implementation.

@9larsons 9larsons merged commit 9d9a421 into main May 20, 2024
22 checks passed
@9larsons 9larsons deleted the posts-contentApi-allowlist branch May 20, 2024 13:25
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.

None yet

3 participants