Skip to content

Refactor internal Query schema and introduce WhereBuilder - #4082

Merged
macobo merged 37 commits into
masterfrom
api-new-query-schema
May 14, 2024
Merged

Refactor internal Query schema and introduce WhereBuilder#4082
macobo merged 37 commits into
masterfrom
api-new-query-schema

Conversation

@macobo

@macobo macobo commented May 8, 2024

Copy link
Copy Markdown
Contributor

Changes

This PR changes:

  1. how we store filters internally in Query object
  2. how we build the WHERE clause of a given query

Motivation is we want to build a saved segments feature and a new version of the API with a more flexible filters system. This work is a prerequisite for both of these.

WhereBuilder specifically makes how we "compile" a query from filters more explicit and makes supporting and/or/not blocks easier in the future.

Note this is a pure refactor - no user-facing changes are occurring here.

Related PRs: #4066, #4071, #4068

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

Comment thread lib/plausible/google/api.ex Outdated
limit,
filters["page"]
# :BUG: This type of filter does not exist.
Plausible.Stats.Query.get_filter(query, "page")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note: This is already broken, @ukutaht is working on a proper replacement under #4077

@macobo
macobo force-pushed the api-new-query-schema branch from f7b0d5a to 89f9cab Compare May 8, 2024 08:09
@macobo
macobo marked this pull request as ready for review May 8, 2024 08:13
@macobo
macobo requested review from RobertJoonas and ukutaht May 8, 2024 08:14
@macobo
macobo force-pushed the api-new-query-schema branch from 89f9cab to ad0e99a Compare May 10, 2024 07:30

@ukutaht ukutaht left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good stuff 👍

Comment thread lib/plausible/stats/filters/where_builder.ex Outdated
Comment thread lib/plausible/stats/filters/where_builder.ex Outdated
Comment thread lib/plausible/stats/table_decider.ex Outdated
site = conn.assigns[:site]
params = Map.put(params, "property", "visit:referrer")

referrer_filter = DashboardFilterParser.filter_value("visit:source", referrer)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is guaranteed by the frontend to be a standard is filter so I think the DashboardFilterParser can be bypassed here. We can use ["is", "visit:source", referrer] instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the "value" is still serialized. The previous put_filter method had special-case parsing logic for just this route - I decided to move it here instead of having it in the general Query class as it's not needed in any other case currently.

@macobo macobo mentioned this pull request May 13, 2024
4 tasks

@RobertJoonas RobertJoonas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

Comment thread lib/plausible_web/controllers/api/stats_controller.ex Outdated
Comment on lines +737 to +741
referrer_filter = DashboardFilterParser.filter_value("visit:source", referrer)

query =
Query.from(site, params)
|> Query.put_filter("visit:source", referrer)
|> Query.put_filter(referrer_filter)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unrelated comment in context of this PR, but might be relevant later on:

I think referrer as a path parameter in this endpoint doesn't make sense anymore - it did when we only supported :is filters, but we might want to show a referrer URL breakdown when multiple sources are filtered by as well.

In any case, that's up to the React dashboard to decide if we should display referrers or sources when a source :member filter is applied. Currently we're still showing sources in that case.

I believe we should delete this endpoint, and start using PlausibleWeb.Api.StatsController.referrers/2 in the React dashboard instead. The source filter should already be included in the Query automatically in that case.

@RobertJoonas
RobertJoonas force-pushed the api-new-query-schema branch from d7b8677 to d7f64d6 Compare May 13, 2024 18:25
@macobo
macobo merged commit baa9965 into master May 14, 2024
@macobo
macobo deleted the api-new-query-schema branch May 14, 2024 08:58
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.

3 participants