feat: add catalogSchemas in Config,catalog_schema_id in Model, Application, ToolSet (URI reference to a catalog schema), catalog_properties in Model, Application, ToolSet - #1111
Conversation
catalogSchemas in Config (map of catalog schema objects for marketplace/catalog display metadata validation)<br>- catalog_schema_id in Model, Application, ToolSet (URI reference to a catalog schema)<br>- catalog_properties in Model, Application, ToolSet (curated marketplace/catalog display metadata, validated against catalog_schema_id)catalogSchemas in Config,catalog_schema_id in Model, Application, ToolSet (URI reference to a catalog schema), catalog_properties in Model, Application, ToolSet
…talogSchemaEntity.sql
This comment has been minimized.
This comment has been minimized.
| try { | ||
| return uriString == null ? null : new URI(uriString); | ||
| } catch (URISyntaxException e) { | ||
| throw new IllegalArgumentException("Invalid application applicationTypeSchemaId: " + uriString); |
There was a problem hiding this comment.
applicationTypeSchemaId or catalogSchemaId?
| @Mapping(target = "toolSets", ignore = true) | ||
| public abstract CatalogSchemaEntity update(CatalogSchema catalogSchema, @MappingTarget CatalogSchemaEntity entity); | ||
|
|
||
| @Mapping(target = "createdAt", source = "createdAt") |
There was a problem hiding this comment.
Looks like those mappings are redundant
| try { | ||
| return uriString == null ? null : new URI(uriString); | ||
| } catch (URISyntaxException e) { | ||
| throw new IllegalArgumentException("Invalid application applicationTypeSchemaId: " + uriString); |
There was a problem hiding this comment.
applicationTypeSchemaId or catalogSchemaId?
| try { | ||
| return uriString == null ? null : new URI(uriString); | ||
| } catch (URISyntaxException e) { | ||
| throw new IllegalArgumentException("Invalid application applicationTypeSchemaId: " + uriString); |
There was a problem hiding this comment.
applicationTypeSchemaId or catalogSchemaId?
| private Long createdAt; | ||
| private Long updatedAt; | ||
|
|
||
| public enum TypeEnum { |
There was a problem hiding this comment.
Why is not just Type?
| Collection<ExportComponentInfo> interceptors = interceptorExporter.preview(request); | ||
| Collection<ExportComponentInfo> interceptorRunners = interceptorRunnerExporter.preview(request); | ||
| Collection<ExportApplicationTypeSchemaInfo> applicationRunners = applicationTypeSchemaExporter.preview(request); | ||
| var globalSettings = globalSettingsExporter.getGlobalSettings(request); |
There was a problem hiding this comment.
Shouldn't catalog schemas be previewed?
|
|
||
| @Override | ||
| Set<ExportConfigComponentType> getDirectDependencies(ExportFormat exportFormat) { | ||
| return Set.of(); |
There was a problem hiding this comment.
As far as I understand catalog schema should be added into direct dependencies of model, application, toolset
| mapper.update(catalogSchema, entity); | ||
| jpaRepository.save(entity); | ||
|
|
||
| log.info("Created catalog schema with id: {}", schemaId); |
There was a problem hiding this comment.
Is that really needed, at least under info level? Related to all other logs below
|
|
||
| @Transactional | ||
| public void create(CatalogSchema catalogSchema) { | ||
| String schemaId = catalogSchema.getSchemaId(); |
There was a problem hiding this comment.
Shouldn't validation similar to app schema be added?
|
|
||
| @Transactional | ||
| public String update(String id, CatalogSchema catalogSchema, String hash) { | ||
| CatalogSchemaEntity entity = jpaRepository.findById(id) |
There was a problem hiding this comment.
Shouldn't validation similar to app schema and hash null check be added?
| .orElseThrow(() -> new EntityNotFoundException(NOT_FOUND_MESSAGE_TEMPLATE.formatted(id))); | ||
|
|
||
| CatalogSchema existingSchema = mapper.toDomain(entity); | ||
| String expectedHash = calculator.calculateHash(existingSchema); |
There was a problem hiding this comment.
if hash=ANY_HASH there is no need to calculate hash
There was a problem hiding this comment.
updated
| .collect(Collectors.toList()); | ||
| } | ||
|
|
||
| public Optional<CatalogSchema> tryGet(String id) { |
There was a problem hiding this comment.
@Transactional(readOnly = true) is missing
| interceptorRunnerService.rollbackInterceptorRunners(revision); | ||
| globalSettingsService.rollbackGlobalSettings(revision); | ||
| adminSettingsService.rollbackAdminSettings(revision); | ||
| catalogSchemaService.rollbackCatalogSchemas(revision); |
There was a problem hiding this comment.
I believe models, apps, toolsets should respect catalog schemas during rollback (see applications -> app type schemas as example)
| AGENT, | ||
| TOOLSET, | ||
| SKILL, | ||
| INTERCEPTOR |
There was a problem hiding this comment.
INTERCEPTOR is listed here but is not supported, is it okay?
| .build(); | ||
| } | ||
|
|
||
| private void resolveDependencies(SelectedItemsExportRequest request) { |
There was a problem hiding this comment.
model, app, toolset should respect catalog schema during custom export
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| private CatalogSchema fromJson(String jsonSchema) { |
There was a problem hiding this comment.
Why is it not done similar to app schemas?
|
|
||
| var schema = schemaWithHash.model(); | ||
| var coreCatalogSchema = schemaCoreMapper.mapToCoreString(schema); | ||
| boolean isSchemaValid = SchemaConformToMetaSchemaValidator.isValid(coreCatalogSchema); |
There was a problem hiding this comment.
Is that correct validation?
There was a problem hiding this comment.
Updated
| @Mapping(target = "updatedAt", ignore = true) | ||
| CatalogSchemaDto toDto(CoreCatalogSchema coreCatalogSchema); | ||
|
|
||
| @Mapping(target = "id", source = "id") |
There was a problem hiding this comment.
Redundant mapping
| import org.mapstruct.Mapping; | ||
|
|
||
| @Mapper(componentModel = "spring", uses = {InstantMapper.class}) | ||
| public interface CoreCatalogSchemaMapper { |
There was a problem hiding this comment.
What is the purpose of this mapper?
There was a problem hiding this comment.
deleted
| return entitySyncStateDtoMapper.toDto(syncState); | ||
| } | ||
|
|
||
| public void create(@Valid CatalogSchemaDto schemaDto) { |
There was a problem hiding this comment.
Those @Valid annotations won't have effect
| ); | ||
|
|
||
| CREATE TABLE catalog_schema_entity_aud ( | ||
| schema_id VARCHAR(512) NOT NULL, |
There was a problem hiding this comment.
Why 512 if main table has 850?
|
|
||
| private String title; | ||
|
|
||
| @Column(length = 2048) |
There was a problem hiding this comment.
Is length limit really needed here?
|
|
||
| @Data | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @CatalogPropertiesConformToSchemas(message = "All deployments with catalog_schema_id should conform to their catalog schema") |
There was a problem hiding this comment.
There can be the case when imported deployment refers to already existing catalog schema, which is missing in imported config. See how it was done for schemas c1d1fb1
There was a problem hiding this comment.
updated
| title text, | ||
| description text, | ||
| catalog_entity_type varchar(50), | ||
| catalog_display_name varchar(512), |
There was a problem hiding this comment.
May be also text as for app schemas?
|
|
||
| ALTER TABLE application_entity ADD COLUMN catalog_schema_id varchar(850); | ||
| ALTER TABLE application_entity ADD COLUMN catalog_properties text; | ||
| ALTER TABLE application_entity ADD CONSTRAINT fk_application_catalog_schema FOREIGN KEY (catalog_schema_id) REFERENCES catalog_schema_entity(schema_id); |
There was a problem hiding this comment.
Why foreign key name is in lower case here and below whereas above is in upper case?
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Applicable issues
catalogSchemaIdin Deployment (String reference to a catalog schema),catalogPropertiesin Deployment (curated marketplace/catalog display metadata, validated against catalogSchemaId) #1097Description of changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.