Ashhar Ahmad Khan created FINERACT-2785:
-------------------------------------------
Summary: EmailDataValidator validates against
ScheduledEmailConstants instead of EmailApiConstants
Key: FINERACT-2785
URL: https://issues.apache.org/jira/browse/FINERACT-2785
Project: Apache Fineract
Issue Type: Bug
Reporter: Ashhar Ahmad Khan
*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.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
