Fix/ans spec compliance#34
Conversation
…set explicit PRIVATE visibility when @notification.customizable is absent
… and move to assemblers package
…ationTypeAutoProvisionerHandler
…d validation and ANS spec compliance
…Type re-provisioning to avoid ANS 400 rejection when existing type has Templates but new payload uses Translations, and align NotificationTemplate re-provisioning to use the same strategy for consistency
…cationType Translations instead of @notification.template.title, and throw on missing required annotation
…tionTemplate re-provisioning
…NotificationTemplate re-provisioning" This reverts commit 8590d10.
busehalis-sap
force-pushed
the
fix/ans-spec-compliance
branch
from
1a3c5b7 to
a98815b
Compare
…ranslations instead of embedded Templates
…s not part of the ANS API spec
…ingTest as DisplayName is no longer set
lisajulia
left a comment
lisajulia
left a comment
There was a problem hiding this comment.
Thanks for the PR :) please have a look at my findings!
| // Visibility - from @notification.customizable annotation (ANS defaults to PRIVATE) | ||
| // @notification.customizable: true → PUBLIC, absent or false → PRIVATE (default) | ||
| String visibility = extractVisibility(event); | ||
| if (visibility != null) { |
There was a problem hiding this comment.
Is this check not needed anymore?
There was a problem hiding this comment.
The default value from ANS is PRIVATE when no visibility is set. I updated the extractVisibility method to handle this explicitly: if the @notification.customizable annotation is absent, it now returns "PRIVATE" directly instead of null. So the method always returns either "PUBLIC" or "PRIVATE", making the null check here unnecessary.
| e.getMessage()); | ||
| } | ||
|
|
||
| createNotificationType(notificationType); |
There was a problem hiding this comment.
So updating a nofication type works like this: updateNotificationType calls DELETE and then createNotficationtype.
What happens if the DELETE fails, then the code will print a warning, failed to delete and try that again - I think we can have an infinite loop here... please add a maxCall or sth. else to prevent this.
If I'm wrong, add at least a test for this, i.e. failing on DELETE.
There was a problem hiding this comment.
Thanks for the hint! Just to clarify the history: this was originally using PATCH, but i switched to delete+create in this PR because existing types in ANS had embedded Templates, and the ANS spec does not allow mixing Translations with Templates on an existing type, so PATCH was being rejected with 400. However, delete+create introduces the infinite loop risk you mentioned. Since all types will have Translations after the first successful provisioning, the Templates mixing issue only occurs during initial migration from old types. For that, old types simply need to be deleted manually from ANS once. I have now switched back to PATCH, which is safer and cleaner: no delete risk, no recursive call chain.
| e.getMessage()); | ||
| } | ||
|
|
||
| createTemplate(template); |
There was a problem hiding this comment.
Can this become an infinite loop? Same problem as in NotificationTypeAutoProvisionerHandler.
There was a problem hiding this comment.
You're right that there's a loop risk here. I explored alternatives but had to stick with delete+create:
Unlike NotificationType, ANS exposes only a PUT endpoint for NotificationTemplate updates, no PATCH. CDS Update.entity().data() is translated to PATCH on remote OData v2 services, so ANS rejects it with 501 Not Implemented. I also tried Upsert.into().entry() but it's not supported on remote OData services either (No ON handler completed the processing). There's no CDS statement that translates to PUT for remote OData services.
So delete+create remains the only viable option here. To prevent the infinite loop, I broke the recursive chain in createTemplate: when a 409 is encountered, instead of calling updateTemplate (which would DELETE+CREATE again and could 409 forever if DELETE keeps failing), i now log a warning and skip. The template stays as-is in ANS for that startup and will be retried on the next one.
|
|
||
| Set<Locale> filtered = new LinkedHashSet<>(); | ||
| for (Locale locale : allLocales) { | ||
| // Always keep root ("und" = fallback for unknown locales) and English |
There was a problem hiding this comment.
What is word "und" in this case?
There was a problem hiding this comment.
'und' is the BCP-47 tag which stands for "undefined/undetermined language", produced by Locale.ROOT.toLanguageTag() in Java. CDS represents the default i18n.properties file (without a language suffix) as Locale.ROOT, which results in 'und'.
Since 'und' is not a valid ANS language code, I have now mapped Locale.ROOT to Locale.ENGLISH ('en') before building translations. Thanks for the hint!
| return translations; | ||
| } | ||
|
|
||
| private static String truncate(String value, int maxLength) { |
There was a problem hiding this comment.
Possibly expose a warning here if some things are truncated?
There was a problem hiding this comment.
Thanks for the suggestion! I tested this with a value exceeding 256 characters. ANS returns a 400 Bad Request which causes a startup failure with a clear error message including the ANS validation details. I decided to keep the explicit failure instead of silent truncation, because truncating would send a shorter value than what the developer defined without them being aware of it. So I removed the truncate method and also improved the 400 error message to mention that field values may exceed ANS length limits, so the developer gets a clearer hint on what to fix.
| notificationType.setNotificationTypeId(notificationTypeId); | ||
| logger.debug( | ||
| "Updating NotificationType '{}' (id={})", | ||
| "Updating NotificationType '{}' (id={}) via delete+create", |
There was a problem hiding this comment.
Did you mean to change the log message from 'via delete+create' to 'via delete+recreate'?
…format, batch index, and email body rendering
…ForEvent as 'und' is not a valid ANS language code
…back to PATCH to eliminate infinite loop risk
…Ds stable instead of delete+create
… skipping update on 409
2 participants
Summary
This PR aligns the notification provisioning logic with the ANS API specification.
- NotificationType payload now uses Translations instead of embedded Templates: The ANS API requires Translations for user-facing display names and group titles. @notification.template.groupedTitle is now a required annotation, startup fails with a clear error if it is missing.
- Delete+create strategy for re-provisioning: ANS rejects payloads that mix Translations and Templates on an existing type (HTTP 400). Replacing UPDATE with delete+create avoids this rejection for NotificationType. The same strategy is applied to NotificationTemplate for consistency.
- Locale filtering per event: Translations are now built only for locales that have app-specific notification texts, filtering out framework-only locales (e.g. 37 languages from @sap/cds/common) that would produce duplicate English content.
- Explicit PRIVATE visibility on templates: When @notification.customizable is absent, PRIVATE is now set explicitly instead of relying on ANS default behavior.
- Null-safe production mode check: cds.environment.production.enabled now correctly defaults to local mode when the property is not configured (previously unboxing a null Boolean could cause a NullPointerException).
- System.out.println replaced with logger.info in LocalNotificationTypeAutoProvisionerHandler: local mode notification type logs now go through the standard logging framework instead of stdout.
- NotificationBuilderTest renamed to NotificationAssemblerTest and moved to assemblers package: the test class was testing NotificationAssembler, not a builder, so the name and location are now aligned with the class under test.