Required Fields V2 - Conditionally Required Fields

[nick-Ag]

This PR revamps the required property for action input fields such that a destination builder can define conditionally required fields. These builder defined conditional requirements are then compiled into a JSONSchema4 object, which is used to validate each mapping before the perform block is triggered.

SDD: Link

Conditional Field Example:

For example, the Salesforce Lead last_name field is only required when a user is creating a lead. We can now encode that in the definition of the field directly like this:

    last_name: {
      label: 'Last Name',
      description: "The lead's last name. **This is required to create a lead.**",
      type: 'string',
      ...
      required: {
        conditions: [
          {
            fieldKey: 'operation',
            operator: 'is',
            value: 'create'
          }
        ]
      },

This builder defined condition will then be compiled into a JSONSchema like this:

{
  '$schema': 'http://json-schema.org/schema#',
  type: 'object',
  additionalProperties: false,
  properties: {
    operation: {
      title: 'Operation',
	...
    },
	...
    last_name: {
      title: 'Last Name',
	...
    },
	...
  },
  required: [ 'operation' ],
  if: {
    properties: {"operation": {"const": "create"}}
  },
  then: { "required": ["last_name"] }
}

And eventually will validate Payloads such as these:
{ operation: 'create', first_name: 'nick' } // invalid - missing last_name
{ operation: 'create', last_name: 'aguilar' } // valid - required last_name is included
{ operation: 'update', first_name: 'nick' } // valid - last_name is not required

Reviewing this PR:

~2800 lines added:

• 1400 - schema-validation.test.ts.snap - this documents the JSON schema generated for each unit test. This is automated and viewing each of these generated schemas isn't important
• 1100 - schema-validation.test.ts - New unit tests which run various combinations of different conditions a builder can define
• 250 - fields-to-jsonschema.ts - The JSON Schema generation logic

Testing

Deployed successfully to staging. I have updated the Salesforce Lead action's last_name field with a conditional requirement.

As of now these conditions are not implemented in the UI, and I am only expecting new AJV error messages when the conditional requirement is not met.

The conditional requirement is enforced when operation: create but last_name is missing:

When operation: create and last_name is defined, the condition is met and event delivery succeeds:

When operation: update the condition is not enforced and last_name can be undefined:

• Added unit tests for new functionality
• Tested end-to-end using the local server
• [Segmenters] Tested in the staging environment

Merge & Deploy Timing

This PR will be a part of a series of PRs. It should not be merged or deployed before these other PRs are ready.
@joe-ayoub-segment - I will handle the final deploy of this one

app: https://github.com/segmentio/app/pull/22618
action-cli: https://github.com/segmentio/action-cli/pull/196 💬

[joe-ayoub-segment]

hi @nick-Ag - I know this is still in draft, so just out of curiosity what will this do?

  groupConditions: {
    anyOf: ['external_id', 'user_alias', 'email', 'braze_id']
  },

Will it hide or show fields in the UI depending on which fields have a mapping populated?
Or will it perform a runtime validation which will prevent the perform() function from being invoked if none of the fields have a value?

Keen to understand the intent here as I think the second approach would be really valuable.

[nick-Ag]

hi @nick-Ag - I know this is still in draft, so just out of curiosity what will this do?

  groupConditions: {
    anyOf: ['external_id', 'user_alias', 'email', 'braze_id']
  },

Will it hide or show fields in the UI depending on which fields have a mapping populated? Or will it perform a runtime validation which will prevent the perform() function from being invoked if none of the fields have a value?

Keen to understand the intent here as I think the second approach would be really valuable.

Planning on the second approach primarily. I also want the validation to appear in the UX so that users can't save mappings where the group condition has not been met. Looking forward to showing more soon!

[github-actions]

New required fields detected

Warning

Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production.

The following required fields were added in this PR:

• Destination: Salesforce (Actions), Action Field(s):last_name

Add these new fields as optional instead and assume default values in perform or performBatch block.

[joe-ayoub-segment]

Hi @nick-Ag , qq:

• does/should the generated-types.ts file change if conditions are used?
• Can you show examples how to use multiple conditions, with all/any (assuming those use-cases are supported)?

[nick-Ag]

Hi @nick-Ag , qq:

• does/should the generated-types.ts file change if conditions are used?
• Can you show examples how to use multiple conditions, with all/any (assuming those use-cases are supported)?

• The generated-types file won't change, fields which are conditionally required will be optional (ex: last_name?: string). See here
• Those cases are supported, see the unit test here for an example. I've tried to test every possible use-case I could think of in that test file

[nick-Ag]

hi @nick-Ag I can't wait to start using this new feature :). Can you tell me what happens when null, '' and undefined values are passed in to a string field which is conditionally required? Also what if the field is defined as AllowNull: true?

I'm going to make a small update to the PR to handle null values as well, thanks for pointing out this case

[varadarajan-tw]

Fantastic work! Great tests!
Left just one question about nulls.

[joe-ayoub-segment]

hi @nick-Ag. Couple of questions about how this code will behave:

1. what will happen in the following scenario? I assume the UI will not fail validation. However when the event gets sent for processing it will fail? Does this mean that validation on the front end needs to be slightly different to validation that happens when an event is being processed?

{
   ... 
   label: 'last_name'
   conditions: [
          {
            fieldKey: 'operation',
            operator: 'is',
            value: 'create'
          }
    ]
 },
{
   ... 
   label: 'operation'
   default: {
      '@path': '$.propertries.operation'
   }
 }

2. what will the error messages be when validation fails? How useful will they be? Are there any examples you can share? The reason I ask is that JSON schema error messages aren't always that helpful for the end user.

[varadarajan-tw]

Thanks for answering my questions! Nice work! ✨ This will be very useful feature saving us some lines of code and bring in some consistency!

[pooyaj]

Looks great 🙌 This is not part of the scope? right?

    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },

wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )

[nick-Ag]

hi @nick-Ag. Couple of questions about how this code will behave:

1. what will happen in the following scenario? I assume the UI will not fail validation. However when the event gets sent for processing it will fail? Does this mean that validation on the front end needs to be slightly different to validation that happens when an event is being processed?

{
   ... 
   label: 'last_name'
   conditions: [
          {
            fieldKey: 'operation',
            operator: 'is',
            value: 'create'
          }
    ]
 },
{
   ... 
   label: 'operation'
   default: {
      '@path': '$.propertries.operation'
   }
 }

2. what will the error messages be when validation fails? How useful will they be? Are there any examples you can share? The reason I ask is that JSON schema error messages aren't always that helpful for the end user.

1. Really good question here. In this case the UI will consider that a valid mapping and pass validation. However if there is no value at $.properties.operation for the event then the validation will fail at the actions level. The validation between the UI and event delivery is only different in the sense that UI validation checks only the users mapping (before path directives are applied) and the event delivery validation will validate against the final payload (path directives applied to the event).
2. Here's an example of an error message in the case that last_name is missing.

[
	{
		"statusCode": 500,
		"message": "The root value is missing the required field 'last_name'. The root value must match \"then\" schema.",
		"stack": [
			"AggregateAjvError: The root value is missing the required field 'last_name'. The root value must match \"then\" schema.",
...

[nick-Ag]

Looks great 🙌 This is not part of the scope? right?

    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },

wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )

Fantastic question. Based on a unit test I just wrote that case is not handled correctly. Going to circle back and address it before merging. Will consider both preventing it entirely vs. implementing it correctly

[joe-ayoub-segment]

Looks great 🙌 This is not part of the scope? right?

    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },

wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )

Fantastic question. Based on a unit test I just wrote that case is not handled correctly. Going to circle back and address it before merging. Will consider both preventing it entirely vs. implementing it correctly

Nice question @pooyaj . My vote is to implement it if possible (if @nick-Ag can do it!). The reason is that Object fields are a neat way to squeeze a lot of fields into a small amount of page real estate. and they also allow us to locate related fields close together. It would be a missed opportunity if 'required' didn't work on these sub fields.

[nick-Ag]

Looks great 🙌 This is not part of the scope? right?

    address: {
      label: 'Address',
      type: 'object',
      properties: {
        city: {
          label: 'city',
          type: 'string',
          required: [Condition Here]
        },

wondering if we handle it gracefully if someone puts a condition on the required field ( ideal if we can prevent it via typescript )

Fantastic question. Based on a unit test I just wrote that case is not handled correctly. Going to circle back and address it before merging. Will consider both preventing it entirely vs. implementing it correctly

Nice question @pooyaj . My vote is to implement it if possible (if @nick-Ag can do it!). The reason is that Object fields are a neat way to squeeze a lot of fields into a small amount of page real estate. and they also allow us to locate related fields close together. It would be a missed opportunity if 'required' didn't work on these sub fields.

Have implemented that use case & updated the README with some documentation about this feature. This one is ready to go out. Looking for final review @joe-ayoub-segment , @varadarajan-tw , @pooyaj

[joe-ayoub-segment]

hi @nick-Ag .
Can we include an example with dot notation for a reference to an object.property field?

[joe-ayoub-segment]

Reviewed the functionality with @nick-Ag . I'm not the best person to review the code though, so please have someone who understands this part of the codebase do a review.