Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unrelated schemas get merged together #38

Open
juiceo opened this issue Jun 17, 2020 · 5 comments
Open

Unrelated schemas get merged together #38

juiceo opened this issue Jun 17, 2020 · 5 comments

Comments

@juiceo
Copy link

juiceo commented Jun 17, 2020

I'm experiencing an issue where two entirely unrelated schemas are getting merged together.

Details

I'm using this library along with routing-controllers-openapi, so it's still a bit unsure if the issue is with this library or somewhere else. Regardless, here's the bug I'm experiencing:

I have the following class, representing a product:

//Product.ts

@JSONSchema({
	description: 'A product object',
})
export class Product {
	@Type(() => LocaleField)
	@ValidateNested()
	@JSONSchema({
		description: 'The name of the product',
		example: {
			def: 'Road bicycle',
		},
	})
	name: LocaleField;

	@Type(() => ProductVariant)
	@ValidateNested({ each: true })
	@IsOptional()
	@JSONSchema({
		description: 'Variants of this product',
	})
        variants?: ProductVariant[];
    

        /** ...other fields */

	constructor(product: ProductApi) {
            this.name = new LocaleField(product.name);
            this.variants = product.variants?.map(v => new ProductVariant(v))
        
             /** ...other fields */
	}
}

And the sub-types are as follows:

//LocaleField.ts

@JSONSchema({
	description: 'A localized string',
})
export class LocaleField {
	@IsString()
	@JSONSchema({
		description: 'The default value for this string',
	})
	def: string;

	@IsString()
	@IsOptional()
	@JSONSchema({
		description: 'The english translation for this string',
	})
	en?: string;

	constructor(data: LocaleFieldType) {
		this.def = data?.def ?? '';
		this.en = data?.en;
	}
}
//ProductVariant.ts

@JSONSchema({
	description: 'A product variant object',
})
export class ProductVariant {
	@IsString()
	name: string;

	@IsInt()
	price: number;

	constructor(data: ProductVariantType) {
		this.name = data.name;
		this.price = data.price || 0;
	}
}

This configuration should clearly result in two different components being defined in my OpenAPI schema, but the resulting schema looks like this:

image

For some reason, the two classes are merged together.

Additional details

Here is the relevant code I'm using to generate the schema:

//app.ts
import 'reflect-metadata'; // this shim is required
import { Request, Response } from 'express';
import { createExpressServer, getMetadataArgsStorage } from 'routing-controllers';
import { routingControllersToSpec } from 'routing-controllers-openapi';
import { defaultMetadataStorage } from 'class-transformer/storage';
import { validationMetadatasToSchemas } from 'class-validator-jsonschema';

import { ProductController, ShopController } from './controllers';
import { ErrorHandler, ResponseTransformer } from './middlewares';
import { RouteNotFoundError } from './errors';
import { openApiConfig } from './config';

const routingControllersOptions = {
	cors: true,
	controllers: [ProductController, ShopController],
	middlewares: [ErrorHandler],
	interceptors: [ResponseTransformer],
	defaultErrorHandler: false,
};

const app = createExpressServer(routingControllersOptions);
const storage = getMetadataArgsStorage();

const schemas = validationMetadatasToSchemas({
	refPointerPrefix: '#/components/schemas/',
	classTransformerMetadataStorage: defaultMetadataStorage,
});

const spec = routingControllersToSpec(storage, routingControllersOptions, {
	components: { schemas },
	...openApiConfig,
});

app.get('/docs', (req: Request, res: Response) => {
	res.status(200).json(spec);
});

app.all('*', (req: Request, res: Response) => {
	const error = new RouteNotFoundError(
		'This route does not exist, or it may have been deprecated.',
	);
	res.status(404).send({
		status: 'error',
		details: error.toJSON(req.originalUrl),
	});
});

export default app;

One possible cause could be that I'm using getMetadataArgsStorage for generating the actual spec, but defaultMetadataStorage as an argument to validationMetadatasToSchemas, which seems a bit off. I'm unsure what would be the correct way to do this however.

@juiceo
Copy link
Author

juiceo commented Jun 17, 2020

Also, any idea why the generated components have these weird keys instead of the class names?

In the class-validator-jsonschema docs it says that the generated schema should have keys according to the class names (i.e. "BlogPost"), but the schema it generates for me has keys like "d", "M" and "f".

@juiceo
Copy link
Author

juiceo commented Jun 17, 2020

Okay, I think I've figured it out. I'm running this within a cloud-functions setup where there the code is also minified with webpack. If I turn off webpack minification, the generated schema keys correspond to the actual class names and it no longer produces wrongly merged schemas.

However it seems quite error-prone that the library relies on the class names as unique identifiers, would there not be a possibility for conflict even without minification?

@epiphone
Copy link
Owner

Hey, glad you figured it out.

One way to handle minified or duplicate class names would be via JSON Schema's id property. I'd be happy to review a PR if somebody wants to take a stab at it.

@juiceo
Copy link
Author

juiceo commented Jun 24, 2020

Thanks for the help, that could be a good way to solve this. Might be able to take a look at it a bit later 👍

@sinclairnick
Copy link

@juiceo Have you tried this without the constructor? Stabbing in the dark a little bit here, but because decorators often modify the constructor, sometimes providing constructors can interfere with how the decorators work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants