FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants

[AshharAhmadKhan]

Fixes: https://issues.apache.org/jira/browse/FINERACT-2785

What is broken

EmailDataValidator is the only validator wired into /v1/email's CREATE and UPDATE endpoints (confirmed via EmailWritePlatformServiceJpaRepositoryImpl, which calls validator.validateCreateRequest(command) / validator.validateUpdateRequest(command)). It currently checks incoming JSON parameters against ScheduledEmailConstants.CREATE_REQUEST_PARAMETERS / UPDATE_REQUEST_PARAMETERS, constants belonging to a completely different feature, the scheduled/report mailing job system (whose actual validator is ReportMailingJobValidator).

Concretely, EmailMessageAssembler.assembleFromJson() reads groupId, clientId, staffId, emailMessage from the request, the real fields for /v1/email, defined in EmailApiConstants. But EmailDataValidator rejects groupId/clientId/staffId outright with UnsupportedParameterException, because they don't exist in ScheduledEmailConstants's allowed parameter set. It also demands fields that don't apply to this resource at all, like stretchyReportId, startDateTime, name, emailRecipients, emailAttachmentFileFormatId.

Net effect, it is currently impossible to create or update a valid email via /v1/email's public API using the fields it's actually documented to accept.

Why it's broken

Full repo grep confirms EmailDataValidator's own copies of isValidEmail/validateEmailRecipients/validateStretchyReportParamMap have zero callers anywhere, so they're dead code. Strong evidence this class was copy pasted from the scheduled mailing job validator and never adapted for /v1/email.

Changes made

EmailDataValidator.validateCreateRequest() and validateUpdateRequest() now check checkForUnsupportedParameters(...) against EmailApiConstants.CREATE_REQUEST_DATA_PARAMETERS / EmailApiConstants.UPDATE_REQUEST_DATA_PARAMETERS instead of the ScheduledEmailConstants sets.

Removed all field level validation belonging to the scheduled mailing job's fields, since none of it applies to /v1/email, and didn't add any new field level rules in its place (see note below). Both methods now just do a blank JSON check and an unsupported parameter check against the correct constants.

Removed dead imports (ScheduledEmailConstants, ScheduledEmailAttachmentFileFormat, LocalDateTime, DateTimeFormatter, ApiParameterError, DataValidatorBuilder, PlatformApiDataValidationException, ArrayList, List, JsonElement). Left isValidEmail/validateEmailRecipients/validateStretchyReportParamMap untouched, including the imports they still need.

Removed the now dead private method throwExceptionIfValidationWarningsExist(...), since nothing calls it anymore.

Added EmailDataValidatorTest.java (6 tests) proving the fix. Covers valid /v1/email fields no longer throwing on create, unsupported parameters still getting rejected on both create and update, and blank JSON still throwing InvalidJsonException on both.

Note

While working on this I found other three things that I'm still investigating. They will be fixed separately. These are as follows, and they need more proper investigation before they can be done:

EmailApiConstants.CREATE_REQUEST_DATA_PARAMETERS is missing emailSubject, even though EmailMessageAssembler reads it via EmailApiConstants.subjectParamName. Need to check the Swagger docs and git blame before deciding if this is even a bug.

EmailApiConstants.UPDATE_REQUEST_DATA_PARAMETERS only allows emailMessage, so groupId/clientId/staffId work on create but get rejected on update. This is existing behavior, not something this fix introduced, just flagging it since my new tests surfaced it.

EmailMessageAssembler.assembleFromJson() calls EmailMessage.pendingEmail(group, client, staff, null, ...), passing emailCampaign as null. pendingEmail() unconditionally calls .setStatusType(emailCampaign.getStatus()), which will NPE. So /v1/email CREATE is probably broken today for a completely separate reason. Haven't runtime confirmed this yet, will file its own ticket once I do.

Related: #6325 (adds the missing UPDATE command handler for /v1/email, this bug was found while writing integration tests for that PR)

[AshharAhmadKhan]

Hey @adamsaghy , can you please retrigger checks when you get a chance, thanks!

I wrapped CREATE_REQUEST_DATA_PARAMETERS and UPDATE_REQUEST_DATA_PARAMETERS in EmailApiConstants.java with Collections.unmodifiableSet(...), since that fixes the two pre-existing MS_MUTABLE_COLLECTION spotbugs findings that were failing CI. Those fields have been mutable since 2017/2020, so it's unrelated to the actual validator logic in this PR, but it was blocking the merge.

[AshharAhmadKhan]

failure unrelated to my code, please retrigger checks, thanks!

[AshharAhmadKhan]

Hey @adamsaghy , this is also ready for review. Please let me know if you want any changes, thanks a lot!

[adamsaghy]

Why to remove all these validations?

[AshharAhmadKhan]

Why to remove all these validations?

hey Adam, those validations are for the scheduled mailing job resource, not /v1/email. This is mentioned in the JIRA ticket (FINERACT-2785).

EmailMessageAssembler only reads groupId, clientId, staffId, message and emailSubject for this endpoint. The fields being validated here (name, startDateTime, stretchyReportId, emailRecipients, emailAttachmentFileFormatId) all belong to ScheduledEmailConstants, a different resource entirely. None of them exist in EmailApiConstants.

So that validation was never really covering /v1/email input. It's also why the real params were getting rejected as unsupported in the first place.

I did not add new required field validation for the real params here on purpose, to avoid scope creep. Planning to raise that as a separate PR once this one is in.

[adamsaghy]

Why to remove all these validations?

hey Adam, those validations are for the scheduled mailing job resource, not /v1/email. This is mentioned in the JIRA ticket (FINERACT-2785).

EmailMessageAssembler only reads groupId, clientId, staffId, message and emailSubject for this endpoint. The fields being validated here (name, startDateTime, stretchyReportId, emailRecipients, emailAttachmentFileFormatId) all belong to ScheduledEmailConstants, a different resource entirely. None of them exist in EmailApiConstants.

So that validation was never really covering /v1/email input. It's also why the real params were getting rejected as unsupported in the first place.

I did not add new required field validation for the real params here on purpose, to avoid scope creep. Planning to raise that as a separate PR once this one is in.

Thank you for the explanation. Now it makes sense: but I would like you to add it as part of this PR. We dont need a separate PR to fix the validations.

[AshharAhmadKhan]

Why to remove all these validations?

hey Adam, those validations are for the scheduled mailing job resource, not /v1/email. This is mentioned in the JIRA ticket (FINERACT-2785).
EmailMessageAssembler only reads groupId, clientId, staffId, message and emailSubject for this endpoint. The fields being validated here (name, startDateTime, stretchyReportId, emailRecipients, emailAttachmentFileFormatId) all belong to ScheduledEmailConstants, a different resource entirely. None of them exist in EmailApiConstants.
So that validation was never really covering /v1/email input. It's also why the real params were getting rejected as unsupported in the first place.
I did not add new required field validation for the real params here on purpose, to avoid scope creep. Planning to raise that as a separate PR once this one is in.

Thank you for the explanation. Now it makes sense: but I would like you to add it as part of this PR. We dont need a separate PR to fix the validations.

done thanks adam!

[AshharAhmadKhan]

failure unrelated to my code, please retrigger when possible!