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

Replace GQL with NQL (filter plugin) #10105

Closed
18 tasks done
kirrg001 opened this issue Nov 6, 2018 · 5 comments
Closed
18 tasks done

Replace GQL with NQL (filter plugin) #10105

kirrg001 opened this issue Nov 6, 2018 · 5 comments
Assignees
Labels
server / core Issues relating to the server or core of Ghost

Comments

@kirrg001
Copy link
Contributor

kirrg001 commented Nov 6, 2018

Goal

As soon as NQL is feature complete, we would like to replace GQL with NQL in the model layer. Ghost uses a filter plugin to hook into the database query.

Why?

GQL has a couple of bugs - especially with filtering on relations.

e.g. #9950

Strategy

The filter plugin is a little messy currently.

Ghost creates joins after GQL has finished. Ghost should not be responsible to create the joins. We should pass a proper configuration object to NQL and based on this information, NQL attaches the correct queries.

Ghost merges custom filters (from user), default and enforced filters. We need to keep this logic. NQL has to offer an interface.

The filter plugin needs to be touched if we want to replace GQL.

Tasks

Ghost: master

@kirrg001 kirrg001 added server / core Issues relating to the server or core of Ghost refactoring/cleanup labels Nov 6, 2018
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 13, 2018

replace options.where logic with extra filters (see)

Started with this task. It's very important to get rid of options.where, because Ghost generates GQL statements. The functions should generate filters. String filters are independent from the implementation details. The goal is to keep the functionality, but to generate filter strings.

kirrg001 added a commit to kirrg001/Ghost that referenced this issue Nov 13, 2018
refs TryGhost#10105

- `options.where` is an older deprecated logic
- before the filter language was invented, Ghost generates statements for knex
- if we want to replace GQL with NQL, we can't generate these statements
- they are not understood from NQL, because NQL uses mongo JSON
- go through usages and rewrite the statements
- invent `extraFilters` for now
- we need to keep the support for `status` or `staticPages` for now (API requirement)
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 13, 2018

            this._filters.statements = gql.json.replaceStatements(this._filters.statements, {prop: /primary_tag/}, function (statement) {
                statement.prop = 'tags.slug';
                return {
                    group: [
                        statement,
                        {prop: 'posts_tags.sort_order', op: '=', value: 0},
                        {prop: 'tags.visibility', op: '=', value: 'public'}
                    ]
                };
            });

Just analysed this custom filter logic. - pre processed filter logic.

  1. tag -> tags.slug is an alias. Will be covered by supporting/using alias config.
  2. {prop: 'tags.visibility', op: '=', value: 'public'}. This is GQL statement format, which anyway doesn't work with NQL. This belongs to the default filter for tag model dependent on context. e.g. filter=visibility:public.
  3. {prop: 'posts_tags.sort_order', op: '=', value: 0}. This is GQL statement format too. This case needs further testing. e.g. filter=tags.slug:en+visibility:public+posts_tags.sort_order:0?

@kirrg001
Copy link
Contributor Author

 if (gql.json.findStatement(this._filters.statements, {prop: /^tags/, op: 'IN'})) {
   // TODO make this count the number of MATCHING tags, not just the number of tags
    this.query('orderByRaw', 'count(tags.id) DESC');
  }

Looking at this piece.

Meaning

Is only used when using IN operator e.g. tags:[x,y].
Any post which matches both tags should be returned first.

NQL?

This is nothing NQL should handle, because NQL only applies queries based on the filter string.
e.g. you can filter data. It does not have the purpose to order the result.

Ghost ?

There is already a TODO.

// TODO move the order handling to the query building that is currently inside pagination

The pagination plugin has a section to respect order definitions of the target model e.g. options.order or options.orderRaw. I think the order function of the post model needs to be conditional. e.g. if the filter wants to filter by tags using IN we should apply this specific order.

order by count(tags.id) DESC, CASE WHEN posts.status = 'scheduled' THEN 1 WHEN posts.status = 'draft' THEN 2 ELSE 3 END ASC,CASE WHEN posts.status != 'draft' THEN posts.published_at END DESC,posts.updated_at DESC,posts.id

kirrg001 added a commit to kirrg001/Ghost that referenced this issue Nov 14, 2018
kirrg001 added a commit that referenced this issue Nov 15, 2018
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Nov 15, 2018
refs TryGhost#10105

- `options.where` is an older deprecated logic
- before the filter language was invented, Ghost generates statements for knex
- if we want to replace GQL with NQL, we can't generate these statements
- they are not understood from NQL, because NQL uses mongo JSON
- go through usages and rewrite the statements
- invent `extraFilters` for now
- we need to keep the support for `status` or `staticPages` for now (API requirement)
kirrg001 added a commit that referenced this issue Nov 15, 2018
refs #10105

- `options.where` is an older deprecated logic
- before the filter language was invented, Ghost generates statements for knex
- if we want to replace GQL with NQL, we can't generate these statements
- they are not understood from NQL, because NQL uses mongo JSON
- go through usages and rewrite the statements
- invent `extraFilters` for now
- we need to keep the support for `status` or `staticPages` for now (API requirement)
- IMO both shortcuts in the extra filters should be removed in the future

This commit is required for #10159!
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Nov 15, 2018
kirrg001 added a commit that referenced this issue Nov 15, 2018
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Nov 15, 2018
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Dec 9, 2018
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Dec 11, 2018
kirrg001 added a commit that referenced this issue Dec 11, 2018
refs #10105, closes #10108, closes #9950, refs #9923, refs #9916, refs #9574, refs #6345, refs #6309, refs #6158, refs TryGhost/GQL#16

- removed GQL dependency
- replaced GQL with our brand new NQL implementation
- fixed all known filter limitations
- GQL suffered from some underlying filter bugs, which NQL tried to fix
- the bugs were mostly in how we query the database for relation filtering
- the underlying problem was caused by a too simple implementation of querying the relations
- mongo-knex has implemented a more robust and complex filtering mechanism for relations
- replaced logic in our bookshelf filter plugin
- we pass the custom, default and override filters from Ghost to NQL, which then are getting parsed and merged into a mongo JSON object. The mongo JSON is getting attached by mongo-knex.

NQL: https://github.com/NexesJS/NQL
mongo-knex: https://github.com/NexesJS/mongo-knex
@kirrg001
Copy link
Contributor Author

FYI: 5 tasks are left, which are post cleanup tasks

@kirrg001
Copy link
Contributor Author

This was shipped in Ghost 2.8.0.. The post cleanup tasks are done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

2 participants