Merge pull request #112 from hackolade/HCK-3720-avro-fe-refdescriptio…

…n-forwarded-on-the-wrong-lev FE: fix refdescription forwarded on the wrong level
hackolade · Jul 13, 2023 · 19ab6c7 · 19ab6c7
2 parents 2d4b5e4 + 06f8d2b
commit 19ab6c7
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 122 deletions.
diff --git 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,63 +313,78 @@ 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.<string, string>} [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 => {
 	return 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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');