Skip to content

Commit

Permalink
chore: review feedback
Browse files Browse the repository at this point in the history 
- Adjusting logs
- Move call location of `checkForIndirectExamples`
- Use literal regex syntax
- Avoid modifying array parameters
- Linting

Signed-off-by: Dustin Popp <[email protected]>
  • Loading branch information
@dpopp07
dpopp07 committed Dec 24, 2024
1 parent 652e9d4 commit e5f5cad
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 34 deletions.
33 changes: 20 additions & 13 deletions packages/ruleset/src/functions/use-date-based-format.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const {
isObjectSchema,
isStringSchema,
schemaHasConstraint,
validateSubschemas,
validateNestedSchemas,
} = require('@ibm-cloud/openapi-ruleset-utilities');

const {
Expand Down Expand Up @@ -78,12 +78,12 @@ function checkForDateBasedFormat(s, p, apidef) {
const examples = [];
const propertyPath = [];

return validateSubschemas(s, p, (schema, path) => {
logger.debug(`${ruleId}: checking schema at location: ${path.join('.')}`);
// Check for any examples outside of the schema path - they may be in
// request bodies, response bodies, or parameters.
examples.push(...checkForIndirectExamples(p, apidef));

// Check for any examples outside of the schema path - they may be in
// request bodies, response bodies, or parameters.
examples.push(...checkForIndirectExamples(path, apidef));
return validateNestedSchemas(s, p, (schema, path) => {
logger.debug(`${ruleId}: checking schema at location: ${path.join('.')}`);

// We can look at primitive schemas directly but for objects and arrays,
// we need more specialized handling in case we need to find a particular
Expand All @@ -103,10 +103,13 @@ function checkForDateBasedFormat(s, p, apidef) {
logger.debug(
`${ruleId}: adding example for schema at location: ${path.join('.')}`
);
logger.debug(
`${ruleId}: - the example path will be : ${propertyPath.join('.')}`
);

examples.push({
example: schema.example,
examplePath: propertyPath.slice(), // Use a copy to prevent modification.
examplePath: [ ...propertyPath ],
});
}

Expand All @@ -124,13 +127,11 @@ function checkForDateBasedFormat(s, p, apidef) {
}
}

// Use a slice (a copy) of the `propertyPath` array so that the
// invoked function can modify it without modifying the original.
return performValidation(
schema,
path,
apidef,
propertyPath.slice(),
propertyPath,
examples
);
});
Expand Down Expand Up @@ -185,13 +186,17 @@ function performValidation(schema, path, apidef, propertyPath, examples) {

// Check example values for string schemas.
if (isStringSchema(schema)) {
// Make a modifiable copy of the path.
propertyPath = [ ...propertyPath ];

// If this is a property, we need to include its name in the path.
if (isSchemaProperty(path)) {
propertyPath.push(path.at(-1));
}

// Either use the schema example directly or search the list of examples
// for an example object that contains a value for this property.
// !!! look for schemas own example needs to be composition-aware - can use Dan's utility
const exampleValue = schema.example || findExample(propertyPath, examples);
if (exampleValue) {
logger.debug(
Expand Down Expand Up @@ -239,6 +244,7 @@ function findExample(propertyPath, examples) {
'.'
)}`
);
logger.debug(`${ruleId}: property path is : ${propertyPath.join('.')}`);

exampleValue = value;
break;
Expand All @@ -260,9 +266,7 @@ function findExample(propertyPath, examples) {
// This function takes an object, as well as a path to a specific value, and
// recursively parses the object looking for the value at that path. If it
// finds one, the value will be returned. If not, the function will return
// `undefined`. One important note is that the array given as the `pathToValue`
// argument *will* be modified by the logic, so if that is not desired, a copy
// should be passed by the caller (using .slice(), for example).
// `undefined`.
function getObjectValueAtPath(obj, pathToValue) {
// If obj is undefined, there is nothing to process.
if (obj === undefined) {
Expand All @@ -274,6 +278,9 @@ function getObjectValueAtPath(obj, pathToValue) {
return obj;
}

// Make a modifiable copy.
pathToValue = [ ...pathToValue ];

const p = pathToValue.shift();

// Check for sentinel indicating an array.
Expand Down
40 changes: 19 additions & 21 deletions packages/ruleset/src/utils/date-based-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,47 +15,47 @@ function isDateBasedName(name) {
// Check for matching against certain patterns.
const dateBasedNamePatterns = [
// The name `created`.
new RegExp('^created$'),
/^created$/,

// The name `updated`.
new RegExp('^updated$'),
/^updated$/,

// The name `modified`.
new RegExp('^modified$'),
/^modified$/,

// The name `expired`.
new RegExp('^expired$'),
/^expired$/,

// The name `expires`.
new RegExp('^expires$'),
/^expires$/,

// Any name ending in `_at`.
new RegExp('.*_at$'),
/.*_at$/,

// Any name ending in `_on`.
new RegExp('.*_on$'),
/.*_on$/,

// Any name starting with `date_`.
new RegExp('^date_.*'),
/^date_.*/,

// Any name containing `_date_`.
new RegExp('.*_date_.*'),
/.*_date_.*/,

// Any name ending in `_date`.
new RegExp('.*_date$'),
/.*_date$/,

// Any name starting with `time_`.
new RegExp('^time_.*'),
/^time_.*/,

// Any name containing `_time_`.
new RegExp('.*_time_.*'),
/.*_time_.*/,

// Note: not including any name ending in `_time` because it was too easy
// to think of counterexamples. `running_time` in the "movies" API of our
// test document in this project is one of them.

// Any name containing `timestamp`.
new RegExp('.*timestamp.*'),
/.*timestamp.*/,
];

return dateBasedNamePatterns.some(regex => regex.test(name));
Expand All @@ -72,24 +72,22 @@ function isDateBasedName(name) {
function isDateBasedValue(value) {
const regularExpressions = [
// Includes abbreviated month name.
new RegExp('\\b(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\\b'),
/\b(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\b/,

// Includes full month name.
new RegExp(
'\\b(January|February|March|April|May|June|July|August|September|October|November|December)\\b'
),
/\b(January|February|March|April|May|June|July|August|September|October|November|December)\b/,

// Includes date in the format YYYY(./-)MM(./-)DD(T).
new RegExp('\\b\\d{4}[./-](0?[1-9]|1[012])[./-]([012]?[1-9]|3[01])(\\b|T)'),
/\b\d{4}[./-](0?[1-9]|1[012])[./-]([012]?[1-9]|3[01])(\b|T)/,

// Includes date in the format DD(./-)MM(./-)YYYY.
new RegExp('\\b([012]?[1-9]|3[01])[./-](0?[1-9]|1[012])[./-]\\d{4}\\b'),
/\b([012]?[1-9]|3[01])[./-](0?[1-9]|1[012])[./-]\d{4}\b/,

// Includes date in the format MM(./-)DD(./-)YYYY.
new RegExp('\\b(0?[1-9]|1[012])[./-]([012]?[1-9]|3[01])[./-]\\d{4}\\b'),
/\b(0?[1-9]|1[012])[./-]([012]?[1-9]|3[01])[./-]\d{4}\b/,

// Includes time in the format (T)tt:tt:tt (where t can be s/m/h/etc.)
new RegExp('(\\b|T)\\d\\d:\\d\\d:\\d\\d\\b'),
/(\b|T)\d\d:\d\d:\d\d\b/,
];

return regularExpressions.some(r => r.test(value));
Expand Down
103 changes: 103 additions & 0 deletions packages/ruleset/test/rules/use-date-based-format.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4077,6 +4077,109 @@ describe(`Spectral rule: ${ruleId}`, () => {
}
});
});

describe('edge cases', () => {
it('schema has property with same name as sub-schema property', async () => {
const testDocument = makeCopy(rootDocument);
testDocument.components.schemas.Movie.properties.studio = {
type: 'object',
properties: {
made: {
type: 'string',
},
},
example: {
made: '2024-12-19',
},
};

testDocument.components.schemas.Movie.properties.made = {
type: 'string',
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

const expectedPaths = [
'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.studio.properties.made',
'paths./v1/movies.post.responses.201.content.application/json.schema.properties.studio.properties.made',
'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.studio.properties.made',
'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.studio.properties.made',
];

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toBe(expectedExampleMsg);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('schema has example defined within allOf', async () => {
const testDocument = makeCopy(rootDocument);
testDocument.components.schemas.Movie.properties.made = {
type: 'string',
allOf: [
{
example: '2024-12-19',
},
],
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

const expectedPaths = [
'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.made',
'paths./v1/movies.post.responses.201.content.application/json.schema.properties.made',
'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.made',
'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.made',
];

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toBe(expectedExampleMsg);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('schema property has example defined within anyOf sibling', async () => {
const testDocument = makeCopy(rootDocument);
testDocument.components.schemas.Movie.properties.studio = {
type: 'object',
properties: {
made: {
type: 'string',
},
},
anyOf: [
{
example: {
made: '2024-12-19',
},
},
],
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

const expectedPaths = [
'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.studio.properties.made',
'paths./v1/movies.post.responses.201.content.application/json.schema.properties.studio.properties.made',
'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.studio.properties.made',
'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.studio.properties.made',
];

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toBe(expectedExampleMsg);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});
});
});
});
});

0 comments on commit e5f5cad

Please sign in to comment.