[riyaa14]

Description

This PR introduces JexlConfiguration within DirectiveConfig to support enabling or disabling the JEXL allowlist feature dynamically through the Wrangler Directive Configuration API.

Changes

1. Introduced JexlConfiguration and JexlConfigurationDeserializer:

• Encapsulates jexlAllowlistEnabled (boolean flag) and jexlAllowlist (list of JexlAllowlist rules) under the jexlConfiguration field in DirectiveConfig.
• Provides isJexlAllowlistEnabledOrDefault(boolean) to fall back to the system-level default when the flag is omitted.

2. Updated DirectiveConfig and Context Resolution:

• Replaced direct jexlAllowlist field in DirectiveConfig with jexlConfiguration.
• Updated ConfigDirectiveContext to resolve whether JEXL allowlisting is active using jexlConfiguration.isJexlAllowlistEnabledOrDefault(...).

3. Updated DirectivesHandler API Validation & Storage:

• Added validation in DirectivesHandler to allow disabling JEXL allowlisting via the config API while preserving existing allowlist rules if omitted when disabled.
• Corrected placement of Auth checks, should be checked before handling of JEXL feature flag and the checks should be enforced irrespective of whether feature flag is enabled or disabled.
• Updated ConfigStore and callers (Wrangler, DataPrepServiceClient, RequestExtractor, RemoteExecutionTask) to serialize/deserialize JexlConfiguration.

[gemini-code-assist]

Code Review

This pull request refactors the JEXL allowlist configuration by introducing a new JexlConfiguration class that encapsulates both the JEXL allowlist and an enabled flag, updating DirectiveConfig and its deserializers accordingly. The review feedback highlights three potential NullPointerException risks: a missing null check on config.getJexlConfiguration() in ConfigDirectiveContext, a potential null return from configStore.getConfig() in DirectivesHandler, and the use of contains(null) on a list in JexlConfiguration which can throw an exception depending on the list implementation.

[gemini-code-assist]

If config.getJexlConfiguration() is null (which can happen if the configuration is empty or has not been fully initialized/migrated), calling isJexlAllowlistEnabledOrDefault will throw a NullPointerException. We should add a null check to safely default to true when the JEXL configuration is missing.

Suggested change

	public boolean isJexlAllowlistEnabled() {
	return jexlAllowlistEnabled;
	return isJexlAllowlistFeatureFlagEnabled
	&& config.getJexlConfiguration().isJexlAllowlistEnabledOrDefault(true);
	}
	public boolean isJexlAllowlistEnabled() {
	return isJexlAllowlistFeatureFlagEnabled
	&& (config.getJexlConfiguration() == null
	|| config.getJexlConfiguration().isJexlAllowlistEnabledOrDefault(true));
	}

[sahusanket]

Please fix.

[riyaa14]

If the feature flag (isJexlAllowlistFeatureFlagEnabled) is true, getJexlConfiguration() won't be null. This is already handled in DirectivesHandler, we don't allow a null jexlConfiguration - Reference

[gemini-code-assist]

If configStore.getConfig() returns null (e.g., in uninitialized or test environments), calling config.toJson() will throw a NullPointerException. It is safer to default to DirectiveConfig.EMPTY when the config is null.

      DirectiveConfig config = configStore.getConfig();
      if (config == null) {
        config = DirectiveConfig.EMPTY;
      }
      if (!jexlAllowlistEnabled) {
        JsonObject configJson = config.toJson().getAsJsonObject();
        configJson.remove(DirectiveConfig.JEXL_CONFIGURATION_KEY);
        return new ServiceResponse<>(configJson);
      }
      return new ServiceResponse<>(config);

[riyaa14]

This case is not possible, the directive config is never null in database because on dataprep service initialization, we initialize the database with default directive config. Reference - Wrangler PR#1040

[gemini-code-assist]

Using jexlAllowlist.contains(null) can throw a NullPointerException depending on the underlying List implementation (for example, Guava's ImmutableList or Java 9+ List.of() throw NPE on contains(null)). It is safer to iterate through the list to check for null elements.

    if (jexlAllowlist != null) {
      for (JexlAllowlist entry : jexlAllowlist) {
        if (entry == null) {
          throw new IllegalArgumentException("JEXL allowlist cannot contain null entries.");
        }
      }
    }

[riyaa14]

This is not needed at all, removed.

[vsethi09]

Use Primitive data type boolean. No need to annotate with nullable.

[vsethi09]

nit: use List here. Underlying implementation could be ImmutableList.

[vsethi09]

Simplify

[vsethi09]

This is not the correct exception to throw in this case.

Let JsonParseException be propaged.

[sahusanket]

Please fix.

[sahusanket]

nit: change userConfig tojexlConfif

[vsethi09]

jexlAllowlist.contains(null) is not needed as this will be handled in the deserializer.