You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "AshharAhmadKhan (via GitHub)" <gi...@apache.org> on 2026/08/25 11:21:49 UTC
[PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
AshharAhmadKhan opened a new pull request, #6330:
URL: https://github.com/apache/fineract/pull/6330
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5410622130
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5413887416
failure unrelated to my code, please retrigger checks, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5414484507
Hey @adamsaghy , this is also ready for review. Please let me know if you want any changes, thanks a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5429651067
failure unrelated to my code, please retrigger when possible!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on code in PR #6330:
URL: https://github.com/apache/fineract/pull/6330#discussion_r3864135989
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/data/EmailDataValidator.java:
##########
@@ -59,185 +50,39 @@ public EmailDataValidator(final FromJsonHelper fromApiJsonHelper) {
}
/**
- * validate the request to create a new report mailing job
+ * validate the request to create a new email message
*
* @param jsonCommand
* -- the JSON command object (instance of the JsonCommand class)
* @return None
**/
public void validateCreateRequest(final JsonCommand jsonCommand) {
final String jsonString = jsonCommand.json();
- final JsonElement jsonElement = jsonCommand.parsedJson();
if (StringUtils.isBlank(jsonString)) {
throw new InvalidJsonException();
}
final Type typeToken = new TypeToken<Map<String, Object>>() {}.getType();
- this.fromApiJsonHelper.checkForUnsupportedParameters(typeToken, jsonString, ScheduledEmailConstants.CREATE_REQUEST_PARAMETERS);
Review Comment:
Why to remove all these validations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #6330:
URL: https://github.com/apache/fineract/pull/6330
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5427397554
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5428748333
> > > 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](https://issues.apache.org/jira/browse/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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "AshharAhmadKhan (via GitHub)" <gi...@apache.org>.
AshharAhmadKhan commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5455473412
hey @adamsaghy , this is ready for review. Please let me know if you need any changes, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2785: Fix EmailDataValidator to validate against EmailApiConstants instead of ScheduledEmailConstants [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #6330:
URL: https://github.com/apache/fineract/pull/6330#issuecomment-5427410804
> > 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](https://issues.apache.org/jira/browse/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org