Skip to content

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented Aug 27, 2025

Hey @macobo @ukutaht

I experimented a little with steps towards removing /query-internal-test endpoint in favor of something maybe more explicit (aliasing/naming subject to change of course). Some upcoming API tests will require patching now, so extending the JSON schema for this felt a bit excessive.. and that what's made me try if we have ways around it.

Disclaimer: I don't feel super strongly about this. This is also unfinished work - I wanted to get feedback from you first, to see whether it makes sense to even continue like that. I can't decide if this way is particularly better or easier to maintain.

We can leverage process dictionary for global overrides such as "now", which might be a good idea since "now" references may be difficult to pass around, but idk, an explicit map with overrides should be fine as well - as demonstrated with conn.private holding parameters that are normally hidden from the public interface.

I found two places where production code calls :internal schema type to build a query that regular users are not permitted to - I've switched those to :public - proving that the whole concept of public and internal JSON schemas can be removed altogether.

Happy to carry on or close, let me know your thoughts please.

@aerosol aerosol requested review from macobo and ukutaht August 27, 2025 11:13
@@ -15,10 +15,6 @@
"uniqueItems": true,
"description": "List of metrics to query"
},
"date": {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -62,53 +58,6 @@
"type": "boolean",
"default": false,
"description": "If set, returns the total number of result rows rows before pagination under `meta.total_rows`"
},
"comparisons": {
Copy link
Contributor

@macobo macobo Aug 28, 2025

Choose a reason for hiding this comment

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

I think the approach makes sense for removing the date param but not for comparisons.

Idea being that date is mainly a testing tool (what is the date we're testing relative against) vs comparisons being a full-blown feature that we might decide to release in the future. Even if we don't, having it be part of the "struct" allows us to build out and test the feature as if it was part of APIv2.

The :internal schema can be thought of as features that are only available on the dashboard.

Copy link
Member Author

Choose a reason for hiding this comment

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

The :internal schema can be thought of as features that are only available on the dashboard.

Ah, thanks. Maybe we should make that clear via code comments then.

@aerosol aerosol closed this Sep 1, 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