From 06f8d2bdc5ca7e4785a9bf02ab8eca67bd053cf2 Mon Sep 17 00:00:00 2001 From: Mikhail Tseluiko Date: Thu, 13 Jul 2023 10:23:33 +0300 Subject: [PATCH] FE: fix refdescription forwarded on the wrong level --- .../helpers/convertJsonSchemaToAvro.js | 138 ++++++++---------- forward_engineering/helpers/generalHelper.js | 51 +++---- forward_engineering/helpers/udtHelper.js | 2 +- reverse_engineering/api.js | 11 +- .../helpers/getFieldAttributes.js | 22 +-- shared/typeHelper.js | 5 +- 6 files changed, 107 insertions(+), 122 deletions(-) diff --git a/forward_engineering/helpers/convertJsonSchemaToAvro.js b/forward_engineering/helpers/convertJsonSchemaToAvro.js index 442b6a0..27d2f86 100644 --- a/forward_engineering/helpers/convertJsonSchemaToAvro.js +++ b/forward_engineering/helpers/convertJsonSchemaToAvro.js @@ -60,7 +60,7 @@ const getAvroType = type => { }; const convertDescriptionToDoc = schema => { - const description = (schema.$ref && schema.refDescription) || schema.description; + const description = schema.description; if (!description) { return schema; } @@ -164,30 +164,24 @@ const convertType = schema => { return convertPrimitive(schema); case 'number': return convertNumber(schema); - case 'enum': - return convertEnum(schema); case 'bytes': return convertBytes(schema); - case 'fixed': - return convertFixed(schema); case 'map': return convertMap(schema); case 'array': return convertArray(schema); + case 'fixed': + return convertFixed(schema); + case 'enum': + return convertEnum(schema); case 'record': return convertRecord(schema); - default: + default: // type is reference return schema; } }; -const filterSchemaAttributes = schema => { - if (_.isArray(schema)) { - return schema; - } - - return filterAttributes(schema.type)(schema); -}; +const filterSchemaAttributes = schema => filterAttributes(schema, schema.type); const convertMultiple = schema => { const type = filterMultipleTypes(schema.type.map(type => { @@ -256,31 +250,12 @@ const getValuesSchema = properties => { }; const convertFixed = schema => { - const name = schema.name || getDefaultName(); - - const convertedSchema = { + return convertNamedType({ ...schema, - name, + name: schema.name || getDefaultName(), size: _.isUndefined(schema.size) ? 16 : schema.size, ...getLogicalTypeProperties(schema), - }; - - const schemaFromUdt = getUdtItem(name); - if (schemaFromUdt && !schemaFromUdt.isCollectionReference && - compareSchemasByStructure(filterSchemaAttributes(convertedSchema), filterSchemaAttributes(schemaFromUdt.schema) - )) { - return convertSchemaToReference(schema); - } - - if (!schemaFromUdt) { - addDefinitions({ [name]: { - schema: filterSchemaAttributes(convertedSchema), - customProperties: getCustomProperties(getFieldLevelConfig(schema.type), schema), - used: true, - }}); - } - - return convertedSchema; + }); }; const convertArray = schema => { @@ -306,7 +281,7 @@ const convertArray = schema => { }; const handleField = (name, field) => { - const { description, default: defaultValue, order, aliases, ...schema } = field; + const { description, refDescription, default: defaultValue, order, aliases, ...schema } = field; const typeSchema = convertSchema(schema); const udt = getUdtItem(typeSchema); const customProperties = udt?.customProperties || getCustomProperties(getFieldLevelConfig(schema.type), schema); @@ -315,7 +290,7 @@ const handleField = (name, field) => { name: prepareName(name), type: _.isArray(typeSchema.type) ? typeSchema.type : typeSchema, default: !_.isUndefined(defaultValue) ? defaultValue : typeSchema?.default, - doc: description, + doc: field.$ref ? refDescription : description, order, aliases, ...customProperties, @@ -338,30 +313,70 @@ const resolveFieldDefaultValue = (field, type) => { }; const convertRecord = schema => { - const name = schema.name || getDefaultName(); - const convertedSchema = { + return convertNamedType({ ...schema, - name, - type: 'record', + name: schema.name || getDefaultName(), fields: Object.keys(schema.fields || {}).map(name => handleField(name, schema.fields[name])), - }; + }); +}; +/** + * Handler for named types (record, enum, fixed). + * If this type is already defined, compare it with existing definition and return reference to it if they are equal. + * If this type is not defined adds it to definitions. + * + * + * @param {Object} schema + * @param {Object.} [schemaTypeKeysMap] key map for properties on the schema type level which may have + * collisions with the field level. For example: + * + * record field with enum type schema may have default that provides default value for this field + * and also default on the type schema level that provides default value from symbols list + * { + * "name": "enumField", + * "default": "defaultValue", // field default + * "type": { // type schema + * "type": "enum", + * "name": "enumType", + * "default": "symbol1", // type schema default + * "symbols": ["symbol1", "symbol2"] + * } + * } + * + * we use property named "symbolDefault" to distinguish this schema type default from field default + * + * example of usage: { symbolDefault: 'default'} + * key of this property is our custom property name on the schema type level, value is the Avro name of this property + * @returns {Object} + */ +const convertNamedType = (schema, schemaTypeKeysMap = {}) => { + const name = schema.name; const schemaFromUdt = getUdtItem(name); - if (schemaFromUdt && !schemaFromUdt.isCollectionReference && - compareSchemasByStructure(filterSchemaAttributes(convertedSchema), filterSchemaAttributes(schemaFromUdt.schema) - )) { + const isAlreadyDefined = schemaFromUdt && !schemaFromUdt.isCollectionReference; + const schemaTypeSpecificKeys = Object.keys(schemaTypeKeysMap); + + if ( + isAlreadyDefined && + compareSchemasByStructure(schema, schemaFromUdt.schema) + // if schemas are not equal, model is not valid for Avro. There will be a validation error + ) { return convertSchemaToReference(schema); } if (!schemaFromUdt) { addDefinitions({ [name]: { - schema: filterSchemaAttributes(convertedSchema), - customProperties: getCustomProperties(getFieldLevelConfig('record'), schema), + schema: { ...filterSchemaAttributes(schema), ..._.pick(schema, schemaTypeSpecificKeys) }, + customProperties: getCustomProperties(getFieldLevelConfig(schema.type)), used: true, }}); } - return convertedSchema; + return { + ..._.omit(schema, schemaTypeSpecificKeys), + ...schemaTypeSpecificKeys.reduce((props, key) => { + return { ...props, [schemaTypeKeysMap[key]]: schema[key] }; + }, {}), + }; }; const convertPrimitive = schema => { @@ -369,32 +384,7 @@ const convertPrimitive = schema => { }; const convertEnum = schema => { - const name = schema.name || getDefaultName(); - - const convertedSchema = { - ...schema, - name, - }; - - const schemaFromUdt = getUdtItem(name); - if (schemaFromUdt && !schemaFromUdt.isCollectionReference && - compareSchemasByStructure(filterSchemaAttributes(convertedSchema), filterSchemaAttributes(schemaFromUdt.schema) - )) { - return convertSchemaToReference(schema); - } - - if (!schemaFromUdt) { - addDefinitions({ [name]: { - schema: { ...filterSchemaAttributes(convertedSchema), symbolDefault: convertedSchema.symbolDefault }, - customProperties: getCustomProperties(getFieldLevelConfig(schema.type), schema), - used: true, - }}); - } - - return { - ..._.omit(convertedSchema, 'symbolDefault'), - default: convertedSchema.symbolDefault, - }; + return convertNamedType({ ...schema, name: schema.name || getDefaultName() }, { symbolDefault: 'default' }); }; const convertNumber = schema => { diff --git a/forward_engineering/helpers/generalHelper.js b/forward_engineering/helpers/generalHelper.js index 3a6ca9c..0f929c1 100644 --- a/forward_engineering/helpers/generalHelper.js +++ b/forward_engineering/helpers/generalHelper.js @@ -1,4 +1,5 @@ const { dependencies } = require('../../shared/appDependencies'); +const { filterAttributes } = require('../../shared/typeHelper'); let _; let nameIndex = 0; @@ -95,40 +96,30 @@ const convertName = schema => { return { ..._.omit(schema, nameProperties), name: prepareName(schema[nameKey])}; }; +/** + * Compares two schemas by their structure. Equality is determined by comparing critical type properties and fields names (for records). + * + * @param {Object} schema1 + * @param {Object} schema2 + * @returns {Boolean} + */ const compareSchemasByStructure = (schema1, schema2) => { - const propertiesToCompare = ['type', 'name', 'fields', 'items', 'values', 'logicalType', 'precision', 'scale', 'size']; - const removeEmptyValues = obj => { - if (!_.isPlainObject(obj)) { - return obj; - } + schema1 = filterAttributes(schema1, schema1.type); + schema2 = filterAttributes(schema2, schema2.type); + const scalarPropertiesToCompare = ['type', 'name', 'logicalType', 'precision', 'scale', 'size']; - return _.pickBy(obj, value => value !== '' && !_.isUndefined(value)); - }; + const isEqualByProperties = _.isEqual(_.pick(schema1, scalarPropertiesToCompare), _.pick(schema2, scalarPropertiesToCompare)); + if (!isEqualByProperties) { + return false; + } - return _.isEqualWith(removeEmptyValues(schema1), removeEmptyValues(schema2), (schema1, schema2, key) => { - if ( - !_.isUndefined(key) && // if key is undefined, one of the objects is empty - !_.isNumber(key) && // if key is number, it's an array index - !propertiesToCompare.includes(key) - ) { - return true; - } + const hasStructure = schema1.fields || schema2.fields; - if (key === 'fields') { - /* - we don't care much about the exact structure of nested fields, - similarity is enough to detect should we - replace schemas by the same reference or rise a warning in validator - */ - - return ( - _.isArray(schema1) && _.isArray(schema2) && - schema1.length === schema2.length && - _.isEqual(schema1.map(schema => schema.name), schema2.map(schema => schema.name)) - ); - } - //if nothing is returned, _.isEqualWith will compare values by default - }); + if (!hasStructure) { + return true; + } + + return _.isEqual(_.map(schema1.fields, 'name'), _.map(schema2.fields, 'name')); }; module.exports = { diff --git a/forward_engineering/helpers/udtHelper.js b/forward_engineering/helpers/udtHelper.js index f0f9e82..7940258 100644 --- a/forward_engineering/helpers/udtHelper.js +++ b/forward_engineering/helpers/udtHelper.js @@ -151,7 +151,7 @@ const convertNamedTypesToReferences = schema => { const convertSchemaToReference = schema => { _ = dependencies.lodash; - const referenceAttributes = filterAttributes()(_.omit(schema, 'type')); + const referenceAttributes = filterAttributes(_.omit(schema, 'type')); return reorderAttributes({ ...referenceAttributes, type: schema.name }); }; diff --git a/reverse_engineering/api.js b/reverse_engineering/api.js index ef47ec9..0a3ee01 100644 --- a/reverse_engineering/api.js +++ b/reverse_engineering/api.js @@ -91,7 +91,12 @@ const getPackages = (avroSchema, jsonSchemas) => { const inferSchemaNameStrategy = ({ name, namespace, confluentSubjectName, schemaTopic }) => { let splittedSubjectName = (confluentSubjectName || '').split('-').filter(Boolean); const endsWithSchemaType = ['key', 'value'].includes(_.last(splittedSubjectName)); - const startsWithTopic = _.first(splittedSubjectName) === schemaTopic; + const startsWithTopic = _.first(splittedSubjectName) === schemaTopic && schemaTopic !== namespace + '.' + name; + const startsWithNamespace = namespace && _.first(splittedSubjectName)?.startsWith(namespace + '.'); + + if (startsWithNamespace) { + splittedSubjectName[0] = splittedSubjectName[0].slice(namespace.length + 1); + } if (endsWithSchemaType) { splittedSubjectName = splittedSubjectName.slice(0, -1); @@ -109,10 +114,6 @@ const inferSchemaNameStrategy = ({ name, namespace, confluentSubjectName, schema return 'TopicNameStrategy'; } - if (namespace && splittedSubjectName?.startsWith(namespace + '.')) { - splittedSubjectName = splittedSubjectName.slice(namespace.length + 1); - } - const splittedRecordName = [...(name || '').split('-')].filter(Boolean); const recordNameStrategy = _.isEqual(splittedRecordName, splittedSubjectName); diff --git a/reverse_engineering/helpers/getFieldAttributes.js b/reverse_engineering/helpers/getFieldAttributes.js index 727d77a..f05f254 100644 --- a/reverse_engineering/helpers/getFieldAttributes.js +++ b/reverse_engineering/helpers/getFieldAttributes.js @@ -9,18 +9,18 @@ let _; const getFieldAttributes = ({ attributes = {}, type = '' }) => { _ = dependencies.lodash; - return _.flow([ - filterAttributes(type), - setNamespace(type), - setSubtype(type), - setDescriptionFromDoc, - convertDefaultValue, - setDurationSize, - addMetaProperties, - ])(attributes); + let fieldAttributes = filterAttributes(attributes, type); + fieldAttributes = setNamespace(fieldAttributes, type); + fieldAttributes = setSubtype(fieldAttributes, type); + fieldAttributes = setDescriptionFromDoc(fieldAttributes); + fieldAttributes = convertDefaultValue(fieldAttributes); + fieldAttributes = setDurationSize(fieldAttributes); + fieldAttributes = addMetaProperties(fieldAttributes); + + return fieldAttributes; }; -const setNamespace = type => properties => { +const setNamespace = (properties, type) => { if (!isNamedType(type)) { return properties; } @@ -32,7 +32,7 @@ const setNamespace = type => properties => { }; }; -const setSubtype = type => properties => { +const setSubtype = (properties, type) => { if (!properties.logicalType || !['fixed', 'bytes'].includes(type)) { return properties } diff --git a/shared/typeHelper.js b/shared/typeHelper.js index 3047d73..96b0741 100644 --- a/shared/typeHelper.js +++ b/shared/typeHelper.js @@ -37,8 +37,11 @@ const LOGICAL_TYPES_MAP = { const isNamedType = type => NAMED_TYPES.includes(type); -const filterAttributes = type => attributes => { +const filterAttributes = (attributes, type) => { _ = dependencies.lodash; + if (_.isArray(attributes)) { + return attributes; + } if (!LOGICAL_TYPES_MAP[attributes.type]?.includes(attributes.logicalType)) { attributes = _.omit(attributes, 'logicalType');