feat: add new LocalizationValue type for displayName, description, intro fields for models, applications, toolsets, inteseptors - #1117
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| @Override | ||
| public void serialize(LocalizedValue value, JsonGenerator gen, SerializerProvider serializers) throws IOException { | ||
| if (!value.isMap()) { |
There was a problem hiding this comment.
Bug: the serializer collapses any single-entry locale map to a bare string regardless of which locale it is, contradicting LocalizedValue.normalize()'s documented contract of only collapsing the default-locale single-entry case.
Example: displayName = {"fr": "Bonjour"} (default locale is "en") — on any response/export round-trip this is written as plain "Bonjour", silently discarding the "fr" tag. If later re-saved unmodified, the value has silently degraded from an explicit French label into a locale-less plain string.
There was a problem hiding this comment.
This behavior is already implemented in DIAL Core, so we must keep the Admin/client implementation consistent with Core. Please use the Core implementation as the source of truth and align the serializer with it rather than introducing a different serialization contract.
There was a problem hiding this comment.
Are we sure that such behavior is expected? Won't users be confused that after saving single locale value, this locale is lost (at least on UI)?
…layName-version-intro' into feat/Localization-value-for-displayName-version-intro
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| import static com.epam.aidial.cfg.client.mapper.CoreMetadataUtils.parseEncodedVersionedPath; | ||
|
|
||
| @Mapper(componentModel = "spring", uses = {RouteMapper.class}) | ||
| @Mapper(componentModel = "spring", uses = {RouteMapper.class, LocalizedValueClientMapper.class, LocalizedValueMapper.class}) |
There was a problem hiding this comment.
Is LocalizedValueMapper.class used?
| import static com.epam.aidial.cfg.client.mapper.CoreMetadataUtils.parseEncodedVersionedPath; | ||
|
|
||
| @Mapper(componentModel = "spring") | ||
| @Mapper(componentModel = "spring", uses = {LocalizedValueClientMapper.class, LocalizedValueMapper.class}) |
There was a problem hiding this comment.
Is LocalizedValueMapper.class used?
| @Component | ||
| public class LocalizedValueClientMapper { | ||
|
|
||
| public LocalizedValueDto toDto(LocalizedValue domain) { |
There was a problem hiding this comment.
Do we really need this custom mapping? Won't regular mapstruct auto-mapping work?
| .addModule(new ValidationModule()) | ||
| .addModule(new JavaTimeModule()) | ||
| .addModule(new SimpleModule() | ||
| .addSerializer(LocalizedValue.class, new LocalizedValueSerializer()) |
There was a problem hiding this comment.
Can't we put serializer/deserializer right into LocalizedValue class?
| List<ModelEntity> findAllByOrderByDisplayNameAscDisplayVersionAscIdAsc(); | ||
|
|
||
| List<ModelEntity> findByIdInOrderByDisplayNameAscDisplayVersionAscIdAsc(Collection<String> ids); | ||
| List<ModelEntity> findByIdInOrderByDisplayNameAscDisplayVersionAscIdAsc(Collection<String> names); |
There was a problem hiding this comment.
Is this change needed?
| @Component | ||
| public class LocalizedValueCoreMapper { | ||
|
|
||
| public CoreLocalizedValue toCoreLocalizedValue(LocalizedValue domain) { |
There was a problem hiding this comment.
Is this custom mapping really needed? Can't we rely on mapstruct auto-mapping?
| private void assertDisplayNameAndDisplayVersionUnique(String currentApplicationName, Application application) { | ||
| Map<String, LocalizedValue> candidates = applicationJpaRepository.findByDisplayVersion(application.getDisplayVersion()).stream() | ||
| .map(mapper::toDomain) | ||
| .filter(a -> a.getDisplayName() != null) |
There was a problem hiding this comment.
Can we do it in DB?
| return false; | ||
| } | ||
|
|
||
| private boolean isBlank(LocalizedValue value) { |
There was a problem hiding this comment.
Looks like it repeats validation in DisplayFieldsValidator. This logic can be put directly into LocalizedValue class
| return value.isPlain() | ||
| ? StringUtils.isNotBlank(value.getPlainValue()) | ||
| : value.getLocaleMap() != null | ||
| && value.getLocaleMap().values().stream().anyMatch(StringUtils::isNotBlank); |
There was a problem hiding this comment.
Shouldn't we also check that map key should not be blank?
There was a problem hiding this comment.
I also didn't catch why we allow to have blank value for locales, e.g. why we allow displayName = {"en": "", "fr": "Bonjour"}?
| @Component | ||
| public class LocalizedValueMapper { | ||
|
|
||
| public LocalizedValueDto toDto(LocalizedValue domain) { |
There was a problem hiding this comment.
The same as for other mappers: can't we rely on mapstruct auto-mapping?
| import java.util.Map; | ||
|
|
||
| @Getter | ||
| @JsonDeserialize(using = LocalizedValueDeserializer.class) |
There was a problem hiding this comment.
Is this supposed to be serialized/deserialized?
|
|
||
| private String endpoint; | ||
| private String iconUrl; | ||
| private String description; |
There was a problem hiding this comment.
I see that description didn't have validation, but for now I think we should validate that if description != null, then it's valid (otherwise I'm not sure how we should handle e.g.
"description": {"":""}
| @Component | ||
| public class LocalizedValueDtoMapper { | ||
|
|
||
| public LocalizedValueDto toDto(LocalizedValue domain) { |
There was a problem hiding this comment.
The same as for other mappers
3 participants
Applicable issues
Description of changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.