feat(source): deny unknown fields for WITH options

[xxchan]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

related:

• bug: the with clause accepts non-existing option #9450
• proposal: normalize connector WITH properties in protobuf #7034

I think this is a low-hanging fruit to resolve #9450 (or mitigate). After this change, we are able to reject invalid WITH options during CREATE SOURCE. The error is created by meta when creating the properties from the raw HashMap.

#7034 may allow us to validate the options even earlier in frontend, but I don't think it's super necessary after this. Correct me if I'm wrong.

Code changes:

• add a unknown_fields: HashMap<String, String> for each source property struct, and add a flag to decide whether to reject unknown fields when creating them (ConnectorProperties::extract).
  • deny when creating new sources. Can search true) when reviewing.
  • accept when recovering old sources
• move some nested fields to top-level, in order to let serde work better.

Note

All source options are fixed, except cdc and datagen. CDC needs to pass many options directly to debezium. Datagen has very complex config for each data field.

Sinks are not updated, since their code path is different. Leave for next PR.

Checklist

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added test labels as necessary. See details.
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
• All checks passed in ./risedev check (or alias, ./risedev c)
• My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

• My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

• My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

[xxchan]

Wait, is WITH options persisted (and thus this will be a breaking change..)?

[xiangjinwu]

FYI there is an existing effort in handler/create_source.rs to reject unknown fields in WITH and it is problematic: #13362

[xxchan]

FYI there is an existing effort in handler/create_source.rs to reject unknown fields in WITH and it is problematic: #13362

risingwave/src/frontend/src/handler/create_source.rs

Lines 571 to 584 in f92b501

	if !options.is_empty() {
	let err_string = format!(
	"Get unknown options for {:?} {:?}: {}",
	source_schema.format,
	source_schema.row_encode,
	options
	.keys()
	.map(|k| k.to_string())
	.collect::<Vec<String>>()
	.join(","),
	);
	session.notice_to_user(err_string);
	}

Seemingly those check are for format ... encode ... (options), instead of WITH options 🤡

[xiangjinwu]

FYI there is an existing effort in handler/create_source.rs to reject unknown fields in WITH and it is problematic: #13362

Seemingly those check are for format ... encode ... (options), instead of WITH options 🤡

The darker side of the story is, source format ... encode ... is mixed with with options:

risingwave/src/frontend/src/utils/with_options.rs

Lines 235 to 239 in f92b501

	let mut options = with_properties.0.clone();
	if let CompatibleSourceSchema::V2(source_schema) = source_schema {
	options.extend_from_slice(source_schema.row_options());
	}
	Self::try_from(options.as_slice())

[tabVersion]

FYI there is an existing effort in handler/create_source.rs to reject unknown fields in WITH and it is problematic: #13362

risingwave/src/frontend/src/handler/create_source.rs

Lines 571 to 584 in f92b501

	if !options.is_empty() {
	let err_string = format!(
	"Get unknown options for {:?} {:?}: {}",
	source_schema.format,
	source_schema.row_encode,
	options
	.keys()
	.map(|k| k.to_string())
	.collect::<Vec<String>>()
	.join(","),
	);
	session.notice_to_user(err_string);
	}

Seemingly those check are for format ... encode ... (options), instead of WITH options 🤡

Yes, there is some misuse of the two, @Rossil2012 is working on that

[xxchan]

Anyway, I can't think of any reasons not to add the validation here, since serde is the last step. 🤔

Regardless of where the config come from, it should finally be a valid defined config.

[xxchan]

Wait, is WITH options persisted (and thus this will be a breaking change..)?

Just tested this. If we apply this, a meta node with unknown fields persisted will fail to start.

---

Oh, create_source_worker_async is used for startup, while create_source_worker is used for registering new source, so maybe we can treat them differently.

[xxchan]

Since the changes in this PR is very simple and non-invasive, maybe we should wait for other ongoing related refactors to be done first. We can come back to this easily.

[xxchan]

Update: since #13414 updated S3 source options, which has some special handling logic. I'm doing some refactoring before merging this PR.

[xxchan]

Current dependencies on/for this PR:

• main
  • PR refactor(source): remove unnecessary Box for NexmarkProperties #14183
    • PR refactor(source): refactor s3 source WITH options handling #14190
      • PR refactor(connector): Add a dummy trait WithOptions #14175
        • PR feat(source): deny unknown fields for WITH options #13654 👈

This stack of pull requests is managed by Graphite.