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

Log warning when validating a spec against the wrong core schema versions #800

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions shared/src/commands/bundled-schemas.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { checkCoreSchemaVersion } from './bundled-schemas';

jest.mock('./helper.js', () => {
return {
initLogger: () => {
return {
info: jest.fn(),
debug: jest.fn(),
error: jest.fn(),
warn: jest.fn()
};
}
};
});

describe('checkCoreSchemaVersion', () => {
it('Return true for non-core schema URLs', () => {
expect(checkCoreSchemaVersion('https://not-calm.org/spec.json', '2024-10', false))
.toBeTruthy();
});

it('Return true for core schema URLs matching loaded version', () => {
expect(checkCoreSchemaVersion('https://calm.finos.org/calm/draft/2024-10/meta/core.json', '2024-10', false))
.toBeTruthy();
});

it('Return false for core schema URLs that dont match loaded version', () => {
expect(checkCoreSchemaVersion('https://calm.finos.org/calm/draft/2020-01/meta/core.json', '2024-10', false))
.toBeFalsy();
});
});
54 changes: 54 additions & 0 deletions shared/src/commands/bundled-schemas.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { readdirSync } from 'fs';
import { CALM_META_SCHEMA_DIRECTORY } from '../consts';
import { initLogger } from './helper';

const calmSchemaRegex = new RegExp('https://.*/draft/([0-9-]+)/meta/.*');

export function getBundledSchemaVersion(debug: boolean): string {
const logger = initLogger(debug);
try {
const files = readdirSync(CALM_META_SCHEMA_DIRECTORY);
if (files.length > 0) {
return files[0];
}
}
catch (_) {
logger.warn('Error looking up bundled CALM schemas. Disabling checks for incorrect core schema versions.');
return null;
}
logger.warn('Did not find bundled CALM schemas. Disabling checks for incorrect core schema versions.');
return null;
}

/**
* Check whether a Schema URL is a core CALM schema, and if it is, log a warning if it doesn't match the bundled version.
* @param uri The schema URI to check
* @param bundledVersion The CALM schema version that was loaded
* @param debug Whether to log debug info
* @returns true if the schema version is valid, false otherwise
*/
export function checkCoreSchemaVersion(uri: string, bundledVersion: string, debug: boolean) {
const logger = initLogger(debug);

if (bundledVersion === null) {
return true;
}

const matches = calmSchemaRegex.exec(uri);

if (!matches || matches.length < 2) {
// not a CALM core schema
return true;
}

const requestedCoreSchemaVersion = matches[1];
if (requestedCoreSchemaVersion === bundledVersion) {
return true;
}

const warningMessage = `WARNING: attempting to load a core CALM schema with a version (${requestedCoreSchemaVersion}) that was not bundled with the CALM CLI. ` +
'This may produce unexpected errors. ' +
`The bundled version is ${bundledVersion}. `;
logger.warn(warningMessage);
return false;
}
7 changes: 7 additions & 0 deletions shared/src/commands/validate/validate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ jest.mock('../helper.js', () => {
};
});

jest.mock('../bundled-schemas.js', () => {
return {
getBundledSchemaVersion: () => null,
checkCoreSchemaVersion: () => null
};
});

const metaSchemaLocation = 'test_fixtures/calm';
const debugDisabled = false;

Expand Down
4 changes: 4 additions & 0 deletions shared/src/commands/validate/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { ValidationOutput, ValidationOutcome } from './validation.output.js';
import { SpectralResult } from './spectral.result.js';
import createJUnitReport from './output-formats/junit-output.js';
import prettyFormat from './output-formats/pretty-output';
import { checkCoreSchemaVersion, getBundledSchemaVersion } from '../bundled-schemas';

let logger: winston.Logger; // defined later at startup

Expand Down Expand Up @@ -64,9 +65,12 @@ export function formatOutput(

function buildAjv2020(debug: boolean): Ajv2020 {
const strictType = debug ? 'log' : false;
const bundledSchemaVersion = getBundledSchemaVersion(debug);
return new Ajv2020({
strict: strictType, allErrors: true, loadSchema: async (uri) => {
try {
// validate schema version
checkCoreSchemaVersion(uri, bundledSchemaVersion, debug);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this work if organisations want to extend the core calm schema but add their own spectral rules for their extensions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you mean if there is effectively a second set of core schemas - the extensions offered by an org - and I guess they would need versioning too?

I guess what I have here is a pretty narrow definition of a 'core schema'.
The reason we embarked on this was that Jim was doing a validate and getting a ton of weird errors because he was using something still pointing at the first release.

What would you say to instead having it just log a warning if you do validation against a spec not found/registered in the schema directory, instead of all this weirdness with 'core schemas'?

Copy link
Member

@LeighFinegold LeighFinegold Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make a lot more sense to me. One scenario I was also concerned about (although more a future roadmap end state ) is if I am referencing someone else's CALM spec for node information do I care if its not on the same version of core version anyways. I'm not sure we would want to mandate it, but also not sure what the implications would be on our current tooling. It's kind of why on this comment I was indicating that maybe the tooling goes via a defined model but then that also implies every piece of tooling is extendable or its extendable by folks supplying the necessary "my calm schema -> tooling model" interpreters as part of the cli setup.

const response = await fetch(uri);
if (!response.ok) {
throw new Error(`Unable to fetch schema from ${uri}`);
Expand Down
Loading