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.

Sign up for GitHub

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

Jump to bottom

fix(rulesets): required readOnly and writeOnly properties should not … #2573

Merged
merged 2 commits into from
May 3, 2024
Merged

fix(rulesets): required readOnly and writeOnly properties should not … #2573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2352e39
fix(rulesets): example validation for required readOnly and writeOnly…
pplr Jan 10, 2024
ae5e06d
Merge branch 'develop' into fix/read-write-only-examples
mnaumanali94 May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 66 additions & 0 deletions packages/rulesets/src/oas/__tests__/oas2-valid-media-example.test.ts
View file
Open in desktop
Expand Up @@ -45,4 +45,70 @@ testRule('oas2-valid-media-example', [
},
],
},

{
name: 'Ignore required writeOnly parameters on responses',
document: {
swagger: '2.0',
paths: {
'/': {
post: {
responses: {
'200': {
schema: {
required: ['ro', 'wo'],
properties: {
ro: {
type: 'string',
readOnly: true,
},
wo: {
type: 'string',
writeOnly: true,
},
other: {
type: 'string',
},
},
},
examples: {
'application/json': {
other: 'foobar',
ro: 'some',
},
},
},
},
},
},
},
responses: {
foo: {
schema: {
required: ['ro', 'wo', 'other'],
properties: {
ro: {
type: 'string',
readOnly: true,
},
wo: {
type: 'string',
writeOnly: true,
},
other: {
type: 'string',
},
},
},
examples: {
'application/json': {
other: 'foo',
ro: 'some',
},
},
},
},
},
errors: [],
},
]);
152 changes: 152 additions & 0 deletions packages/rulesets/src/oas/__tests__/oas3-valid-media-example.test.ts
View file
Open in desktop
Expand Up @@ -312,6 +312,158 @@ testRule('oas3-valid-media-example', [
errors: [],
},

{
name: 'Ignore required readOnly parameters on requests',
document: {
openapi: '3.0.0',
paths: {
'/': {
post: {
requestBody: {
content: {
'application/json': {
schema: {
required: ['ro', 'wo'],
properties: {
ro: {
type: 'string',
readOnly: true,
},
wo: {
type: 'string',
writeOnly: true,
},
other: {
type: 'string',
},
},
},
example: {
other: 'foobar',
wo: 'some',
},
},
},
},
},
},
},
components: {
requestBodies: {
foo: {
content: {
'application/json': {
schema: {
required: ['ro', 'wo', 'other'],
properties: {
ro: {
type: 'string',
readOnly: true,
},
wo: {
type: 'string',
writeOnly: true,
},
other: {
type: 'string',
},
},
},
examples: {
valid: {
summary: 'should be valid',
value: {
other: 'foo',
wo: 'some',
},
},
},
},
},
},
},
},
},
errors: [],
},

{
name: 'Ignore required writeOnly parameters on responses',
document: {
openapi: '3.0.0',
paths: {
'/': {
post: {
responses: {
'200': {
content: {
'application/json': {
schema: {
required: ['ro', 'wo'],
properties: {
ro: {
type: 'string',
readOnly: true,
},
wo: {
type: 'string',
writeOnly: true,
},
other: {
type: 'string',
},
},
},
example: {
other: 'foobar',
ro: 'some',
},
},
},
},
},
},
},
},
components: {
responses: {
foo: {
content: {
'application/json': {
schema: {
required: ['ro', 'wo', 'other'],
properties: {
ro: {
type: 'string',
readOnly: true,
},
wo: {
type: 'string',
writeOnly: true,
},
other: {
type: 'string',
},
},
},
examples: {
valid: {
summary: 'should be valid',
value: {
other: 'foo',
ro: 'some',
},
},
},
},
},
},
},
},
},
errors: [],
},

{
name: 'parameters: will fail when complex example is used',
document: {
Expand Down
76 changes: 76 additions & 0 deletions packages/rulesets/src/oas/functions/oasExample.ts
View file
Open in desktop
Expand Up @@ -11,6 +11,10 @@ export type Options = {
type: 'media' | 'schema';
};

type HasRequiredProperties = traverse.SchemaObject & {
required?: string[];
};

type MediaValidationItem = {
field: string;
multiple: boolean;
Expand Down Expand Up @@ -39,6 +43,22 @@ const MEDIA_VALIDATION_ITEMS: Dictionary<MediaValidationItem[], 2 | 3> = {
],
};

const REQUEST_MEDIA_PATHS: Dictionary<JsonPath[], 2 | 3> = {
2: [],
3: [
['components', 'requestBodies'],
['paths', '*', '*', 'requestBody'],
],
};

const RESPONSE_MEDIA_PATHS: Dictionary<JsonPath[], 2 | 3> = {
2: [['responses'], ['paths', '*', '*', 'responses']],
3: [
['components', 'responses'],
['paths', '*', '*', 'responses'],
],
};

const SCHEMA_VALIDATION_ITEMS: Dictionary<string[], 2 | 3> = {
2: ['example', 'x-example', 'default'],
3: ['example', 'default'],
Expand All @@ -49,6 +69,22 @@ type ValidationItem = {
path: JsonPath;
};

function hasRequiredProperties(schema: traverse.SchemaObject): schema is HasRequiredProperties {
return schema.required === undefined || Array.isArray(schema.required);
}

function isSubpath(path: JsonPath, subPaths: JsonPath[]): boolean {
return subPaths.some(subPath => subPath.every((segment, idx) => segment === '*' || segment === path[idx]));
}

function isMediaRequest(path: JsonPath, oasVersion: 2 | 3): boolean {
return isSubpath(path, REQUEST_MEDIA_PATHS[oasVersion]);
}

function isMediaResponse(path: JsonPath, oasVersion: 2 | 3): boolean {
return isSubpath(path, RESPONSE_MEDIA_PATHS[oasVersion]);
}

function* getMediaValidationItems(
items: MediaValidationItem[],
targetVal: Dictionary<unknown>,
Expand Down Expand Up @@ -146,6 +182,41 @@ function cleanSchema(schema: Record<string, unknown>): void {
}));
}

/**
* Modifies 'schema' (and all its sub-schemas) to make all
* readOnly or writeOnly properties optional.
* In this context, "sub-schemas" refers to all schemas reachable from 'schema'
* (e.g. properties, additionalProperties, allOf/anyOf/oneOf, not, items, etc.)
* @param schema the schema to be modified
* @param readOnlyProperties make readOnly properties optional
* @param writeOnlyProperties make writeOnly properties optional
*/
function relaxRequired(
schema: Record<string, unknown>,
readOnlyProperties: boolean,
writeOnlyProperties: boolean,
): void {
if (readOnlyProperties || writeOnlyProperties)
traverse(schema, {}, <traverse.Callback>((
fragment,
jsonPtr,
rootSchema,
parentJsonPtr,
parentKeyword,
parent,
propertyName,
) => {
if ((fragment.readOnly === true && readOnlyProperties) || (fragment.writeOnly === true && writeOnlyProperties)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could we also change the schema to false or not: true?
That's to ensure we catch if the property is defined.
To illustrate it with example:

{
  "type": "object",
  "properties": {
    "foo": {
      "type": "string",
      "readOnly": true
    }
  },
  "required": [
    "foo"
  ]
}

The above schema would be transformed to

{
  "type": "object",
  "properties": {
    "foo": false
  }
}

This way we wouldn't let property "foo" be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @P0lip, do you request to make readOnly / writeOnly properties forbidden on request / response? Not sure that this is the exact definition of the read and writeOnly flags.

The 3.0 OAS spec says "SHOULD NOT".

For OAS 3.1 / json schema, the readOnly / writeOnly definition is more open.

If readOnly has a value of boolean true, it indicates that the value of the
instance is managed exclusively by the owning authority, and attempts by an
application to modify the value of this property are expected to be ignored or
rejected by that owning authority.

Your proposal could be optional and enabled with a flag like 'strict-read-write-only' ?

if (parentKeyword == 'properties' && parent && hasRequiredProperties(parent)) {
parent.required = parent.required?.filter(p => p !== propertyName);
if (parent.required?.length === 0) {
delete parent.required;
}
}
}
}));
}

export default createRulesetFunction<Record<string, unknown>, Options>(
{
input: {
Expand Down Expand Up @@ -190,6 +261,11 @@ export default createRulesetFunction<Record<string, unknown>, Options>(
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
schemaOpts.schema = JSON.parse(JSON.stringify(schemaOpts.schema));
cleanSchema(schemaOpts.schema);
relaxRequired(
schemaOpts.schema,
opts.type === 'media' && isMediaRequest(context.path, opts.oasVersion),
opts.type === 'media' && isMediaResponse(context.path, opts.oasVersion),
);

for (const validationItem of validationItems) {
const result = oasSchema(validationItem.value, schemaOpts, {
Expand Down