v3.2.2 RC (#134)

* Adding a [service, version] existence validation and returning an HTTP 400 Bad Request in case the combination exists. (#79) * Adding a [service, version] existence validation and returning an HTTP 400 Bad Request in case the combination exists. * Adding a functional test case. Co-authored-by: Artjom Kurapov <[email protected]> * add api test for parallel requests against /schema/push * update docs, bump changelog Co-authored-by: Etienne Hardy <[email protected]>
tot-ra · May 24, 2022 · 4b34836 · 4b34836
1 parent c322592
commit 4b34836
Show file tree

Hide file tree

Showing 5 changed files with 115 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,17 +5,31 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+Types of changes:
+- Added for new features.
+- Changed for changes in existing functionality.
+- Deprecated for soon-to-be removed features.
+- Removed for now removed features.
+- Fixed for any bug fixes.
+- Security in case of vulnerabilities.
+
 ## [Unreleased]
 
+## [3.2.3] - 2022-05-25
+## Fixed
+- /schema/push now returns 400 instead of 500 if you attempt to register service schema with same version
+- note, that you can register schema from multiple pods if it is the same (type definitions are matched)
+
 ## [3.2.2] - 2022-05-24
+
 ## Updated
+
 - Fonts in diff & definition tabs are now monospace & same size
 - Added support for use `TypeScript` in the client side code. A pair of components were converted to TypeScript to validate the configuration.
 - Added the configuration for `testing-library/react` and `testing-library/react-hooks`, so unit tests could be added to the client side code. A pair of tests were added to validate the configuration.
 - `eslint-webpack-plugin` was added to avoid forgetting to fix eslint and prettier issues during development.
 - Some commands on Dockerfiles were rewritten to benefit from caching when executing `npm install`
 
-
 ## [3.2.1] - 2022-05-14
 
 ### Updated
@@ -263,7 +277,8 @@ DELETE /schema/:schemaId
 - Frontend app
 - Examples of gateway + 2 federated services
 
-[unreleased]: https://github.com/pipedrive/graphql-schema-registry/compare/v3.2.2...HEAD
+[unreleased]: https://github.com/pipedrive/graphql-schema-registry/compare/v3.2.3...HEAD
+[3.2.3]: https://github.com/pipedrive/graphql-schema-registry/compare/v3.2.2...v3.2.3
 [3.2.2]: https://github.com/pipedrive/graphql-schema-registry/compare/v3.2.1...v3.2.2
 [3.2.1]: https://github.com/pipedrive/graphql-schema-registry/compare/v3.2.0...v3.2.1
 [3.2.0]: https://github.com/pipedrive/graphql-schema-registry/compare/v3.1.0...v3.2.0

diff --git a/README.md b/README.md
@@ -250,12 +250,17 @@ See main [blog post](https://medium.com/pipedrive-engineering/journey-to-federat
 
 ### GET /schema/latest
 
-Simplified version of /schema/compose where latest versions from different services is composed. Needed mostly for debugging
+Simplified version of /schema/compose where latest versions from different services are composed.
+Some services prefer this to use this natural schema composition, as its natural and time-based.
 
 ### POST /schema/compose
+Advanced version of schema composition, where you need to provide services & their versions.
+Used by graphql gateway to fetch schema based on currently running containers.
 
-Lists schema based on passed services & their versions.
-Used by graphql gateway to fetch schema based on current containers
+The advantage over time-based composition is that versioned composition allows to automatically
+update federated schema when you deploy older version of the pod in case of some incident.
+If you deploy older pods they can ofc try to register old schema again, but as it already exists
+in schema-registry, it will not be considered as "latest".
 
 #### Request params (optional, raw body)
 
@@ -333,6 +338,7 @@ URL is optional if you use urls from schema-registry as service discovery
   "url": "http://service-b.develop.svc.cluster.local"
 }
 ```
+- ❌ 400 "You should not register different type_defs with same version."
 
 #### POST /schema/validate
 

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "graphql-schema-registry",
-  "version": "3.2.2",
+  "version": "3.2.3",
   "description": "Graphql schema registry",
   "main": "app/schema-registry.js",
   "scripts": {

diff --git a/src/database/schema.ts b/src/database/schema.ts
@@ -2,6 +2,7 @@ import Knex from 'knex';
 import { unionBy } from 'lodash';
 import { connection } from './index';
 import servicesModel from './services';
+import { PublicError } from '../helpers/error';
 import { logger } from '../logger';
 
 function isDevVersion(version: string) {
@@ -217,6 +218,12 @@ const schemaModel = {
 			})
 		)[0]?.id;
 
+		if (existingService && !schemaId && await versionExists(trx, existingService.id, service.version)) {
+			const message = `Schema [${existingService.name}] and version [${service.version}] already exist in registry. `
+				+ `You should not register different type_defs with same version.`;
+			throw new PublicError(message, null);
+		}
+
 		if (schemaId) {
 			await trx('schema')
 				.where('id', '=', schemaId)
@@ -385,4 +392,13 @@ const schemaModel = {
 	},
 };
 
+async function versionExists(trx, serviceId, version) {
+	return (
+		await trx('container_schema').select('id').where({
+			service_id: serviceId,
+			version: version,
+		})
+	)[0]?.id;
+}
+
 export default schemaModel;
diff --git a/test/functional/schema/push.test.ts b/test/functional/schema/push.test.ts
@@ -61,4 +61,76 @@ describe('POST /schema/push', function () {
 			);
 		}
 	});
+
+	it('allows to register same schema from same service twice, because multiple pods can start at the same time', async () => {
+		let result = await request({
+			method: 'POST',
+			uri: 'http://localhost:6001/schema/push',
+			resolveWithFullResponse: true,
+			json: true,
+			body: {
+				name: 'service_c',
+				version: 'v1',
+				type_defs: '\n\ttype Query {\n\t\tme: String\n\t}\n',
+			},
+		});
+
+		expect(result.statusCode).toBe(200);
+
+		try {
+			result = await request({
+				method: 'POST',
+				uri: 'http://localhost:6001/schema/push',
+				resolveWithFullResponse: true,
+				json: true,
+				body: {
+					name: 'service_c',
+					version: 'v1',
+					type_defs: '\n\ttype Query {\n\t\tme: String\n\t}\n',
+				},
+			});
+		} catch (err) {
+			expect(err).toBe('unexpected error');
+		}
+
+		expect(result.statusCode).toBe(200);
+	});
+
+	it('returns 400 if new schema definition gets pushed with an existing version', async () => {
+		let result = await request({
+			method: 'POST',
+			uri: 'http://localhost:6001/schema/push',
+			resolveWithFullResponse: true,
+			json: true,
+			body: {
+				name: 'service_c',
+				version: 'v1',
+				type_defs: '\n\ttype Query {\n\t\tme: String\n\t}\n',
+			},
+		});
+
+		expect(result.statusCode).toBe(200);
+
+		try {
+			 result = await request({
+				method: 'POST',
+				uri: 'http://localhost:6001/schema/push',
+				resolveWithFullResponse: true,
+				json: true,
+				body: {
+					name: 'service_c',
+					version: 'v1',
+					type_defs: '\n\ttype Query {\n\t\tme: String\n\tyou:String\n\t}\n',
+				},
+			});
+		} catch (err) {
+			expect(err.statusCode).toBe(400);
+			expect(err.response.body).toEqual(
+				expect.objectContaining({
+					success: false,
+					message: expect.stringContaining('You should not register different type_defs with same version')
+				})
+			);
+		}
+	});
 });