Improve query registration and schema validation #243

chriszarate · 2024-12-13T21:54:09Z

Improve schema validation and sanitization with formal Type grammar.
- Address the limitation with the current schema system by enabling support for self-referential schemas, allowing for arbitrary nesting of data structures.
- Ensure the system can handle complex data structures without requiring workarounds.
Improve query registration by providing a single entrypoint with associative array config
- Provide a more intuitive method for ad hoc use of remote data blocks, making it easier to execute queries outside the block editor context.
- Remove the dependency on block names as identifiers, allowing for a straightforward function call to execute queries.
- Consolidate the registration process into a single entry point, reducing the need for multiple function calls and improving code clarity.
Provide a "service config" for each data source that can validate itself.
- Instead of serializing the entire config, serialize only the "service config" which can be used to "compute" create the full config. This allows us to make minor changes to the full config without breaking existing configured data sources.

Known issues

~~Registered generic HTTP data sources is broken.~~
~~Psalm validation is broken because of NumberFormatter~~
~~Github markdown source seems to be broken.~~
Any other issues encountered, please report them!

maxschmeling · 2024-12-17T14:42:29Z

inc/Integrations/Airtable/AirtableIntegration.php

-			LoggerManager::instance()->error( 'Failed to get query for Airtable block' );
-			return;
-		}
+	public static function register_block_for_airtable_data_source( AirtableDataSource $data_source, array $block_overrides = [] ): void {


Should we follow the {type}_block naming here and make this something like register_item_block_... or register_single_item_block_... ?

I'm fine with whatever naming scheme. I think we might even want to centralize the entrypoints for registering official integrations, like Integration::register_block( [ /* options */ ] )

maxschmeling · 2024-12-17T14:42:59Z

inc/Integrations/Airtable/AirtableIntegration.php

-			LoggerManager::instance()->error( 'Failed to get query for Airtable block' );
-			return;
-		}
+	public static function register_block_for_airtable_data_source( AirtableDataSource $data_source, array $block_overrides = [] ): void {


Should we follow the {type}_block naming here and make this something like register_item_block_... or register_single_item_block_... ?

maxschmeling · 2024-12-17T14:44:29Z

example/airtable/events/register.php

+	] );
+
+	$block_options = [
+		'pages' => [


Is it necessary for page registration to be part of the block registration? I mainly ask because we might want to have overrides for multiple blocks on a page.

Confirmed you can do this now. The page is only created once if you use the same page slug for multiple blocks.

We really need some deeper product exploration around pages, though, because I don't really know what use cases we are solving.

brookewp · 2024-12-17T17:02:49Z

A couple things I noticed when testing:

When selecting an item from a Shopify block, pattern selection is skipped
Airtable data isn't available:
Edit: looks like this happens when a photo/attachment field is selected

chriszarate · 2024-12-18T04:20:51Z

Thanks @brookewp!

When selecting an item from a Shopify block, pattern selection is skipped

This is intentional when the registered pattern has a role of inner_blocks. This was a requested feature at one point; if I have a pattern I know I want to use, I should skip pattern selection.

remote-data-blocks/src/blocks/remote-data-container/hooks/usePatterns.ts

Line 112 in b2016ca

returnValue.insertPatternBlocks( innerBlocksPattern );

Airtable data isn't available:
Edit: looks like this happens when a photo/attachment field is selected

I believe I've "fixed" this type mismatch issue by selecting the first item in the array. Could you confirm?

shekharnwagh · 2024-12-18T15:27:47Z

Github markdown source seems to be broken. When adding a markdown, it renders the raw markdown code instead of its HTML version like it used to earlier.

chriszarate · 2024-12-18T16:50:39Z

Github markdown source seems to be broken. When adding a markdown, it renders the raw markdown code instead of its HTML version like it used to earlier.

Thanks @shekharnwagh, fixed in 912831c

docs/extending/block-patterns.md

docs/extending/data-source.md

docs/extending/query-runner.md

alecgeatches · 2024-12-19T17:23:04Z

docs/extending/query.md

-
-By default, the `get_endpoint` method proxies to the `get_endpoint` method of query's data source. Override this method to set a custom endpoint for the query—for example, to construct the endpoints using an input variable. The input variables for the current request are provided as an associative array (`[ $var_name => $value ]`).
+- `format` (optional): A callable function that formats the output variable value.
+- `generate` (optional): A callable function that generates or extracts the output variable value from the response, as an alternative to `path`.


I see that this is an alternative to path, but path below is listed as (required). A bit confusing, clarification here would be nice.

Addressed in 5060870

docs/extending/query.md

alecgeatches · 2024-12-19T17:52:06Z

docs/workflows/index.md

+- [Create an Airtable integration](airtable.md)
+- [Create a Google Sheets integration](google-sheets.md)
+- [Create a REST API integration](rest-api.md)
+- [Create a REST API integration with code](rest-api-with-code.md)


Should the top three bullet points be under the "UI-based workflows" header? It's a bit confusing to see "Create a REST API integration" and "Create a REST API integration with code" together here.

Also "with code" / "Code-based" might be the wrong nomenclature since they both utilize code, it's more about the starting point. Maybe "UI-configured sources" versus "Code-generated sources" or something like that?

Addressed in 5060870

docs/workflows/rest-api-with-code.md

alecgeatches · 2024-12-19T18:01:10Z

example/airtable/events/register.php

-	register_remote_data_loop_block( 'Conference Event List', $airtable_list_events_query );
-	register_remote_data_page( $block_name, 'conference-event' );
+	AirtableIntegration::register_block_for_airtable_data_source( $airtable_data_source, $block_options );
+	AirtableIntegration::register_loop_block_for_airtable_data_source( $airtable_data_source, $block_options );


@chriszarate I'm curious why we use these new integration-specific wrappers. Why is that?

Because they automatically provide the queries that are specific to the integration. Definitely open to different naming or organization.

docs/extending/block-registration.md

alecgeatches · 2024-12-19T18:13:11Z

inc/Config/DataSource/HttpDataSource.php

 		}

 		return static::from_array( $config );
 	}

 	/**
 	 * @inheritDoc
-	 * @psalm-suppress ParamNameMismatch reason: we want the clarity provided by the rename here
+	 *
+	 * TODO: Do we need to sanitize this to prevent leaking sensitive data?


I may be misunderstanding the security concern here, but I don't think so. We don't currently expose this in the UI (I think), we just slurp it into ConfigStore or use values in code. If a user can run arbitrary code to dump ::to_array(), they'd also have the ability to read defines or env keys or WP config values anyhow. Am I missing something?

We don't currently expose this in the UI (I think)

@alecgeatches Not visually, but anyone that can access the REST endpoint that supports the plugin settings screen (GET remote-data-blocks/data-sources) can view the full service config, including credentials. Currently this is limited to manage_options but I imagine there will be a push to lower that substantially.

This is currently intentional-by-design, because there is front-end code that needs those credentials in order to call the APIs to do schema discovery. However, we probably need to redact sensitive credentials and conduct these requests via the REST API, so that we don't expose them.

They even make their way into the DOM, since they are used to populate password fields when a user edits an existing data source.

Co-authored-by: Alec Geatches <[email protected]>

maxschmeling

I really like the direction this moves us. I'm sure there will be some tweaks still, but lets get it merged before we have more conflicts! 🚢

chriszarate mentioned this pull request Dec 16, 2024

Improve schema validation and sanitization #237

Closed

chriszarate force-pushed the improve/query-registration branch 3 times, most recently from 8b4be91 to 55bf542 Compare December 17, 2024 14:54

maxschmeling reviewed Dec 17, 2024

View reviewed changes

chriszarate marked this pull request as ready for review December 17, 2024 15:01

chriszarate force-pushed the improve/query-registration branch 2 times, most recently from ea8c354 to b2016ca Compare December 18, 2024 04:12

chriszarate mentioned this pull request Dec 18, 2024

Make QueryRunner an implementation detail and provide execute on public interface #221

Closed

chriszarate force-pushed the improve/query-registration branch from b2016ca to 310297c Compare December 18, 2024 16:49

chriszarate force-pushed the improve/query-registration branch 3 times, most recently from 0f1c17b to c3d6720 Compare December 19, 2024 16:59