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

refactor: the parsing process #85

Merged
merged 4 commits into from
Mar 18, 2022
Merged

refactor: the parsing process #85

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b2aad2
chore: refactor the parsing process
KhudaDad414 Mar 2, 2022
9f323b3
Update src/ComponentProvider.ts
KhudaDad414 Mar 8, 2022
bf8baa7
naming and a new message path
KhudaDad414 Mar 8, 2022
eb1ed84
Merge branch 'master' into parse-improvements
KhudaDad414 Mar 18, 2022
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
53 changes: 35 additions & 18 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"dependencies": {
"@asyncapi/parser": "^1.11.1",
"js-yaml": "^4.1.0",
"jsonpath-plus": "^6.0.1",
"lodash": "^4.17.21",
"merge-deep": "^3.0.3"
}
Expand Down
122 changes: 44 additions & 78 deletions src/ComponentProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,94 +1,60 @@
import type { AsyncAPIDocument, ChannelParameter, Message, MessageTrait, Schema } from '@asyncapi/parser';

import type {
AsyncAPIDocument,
ChannelParameter,
Message,
Schema,
} from '@asyncapi/parser';

import { JSONPath } from 'jsonpath-plus';
import _ from 'lodash';
/**
* This class will provide all sorts of data for optimizers.
*
* @private
*/
export class ComponentProvider {
messagePaths = ['$.channels.*.*.message', '$.components.messages.*'];
KhudaDad414 marked this conversation as resolved.
Show resolved Hide resolved
schemaPaths = [
'$.channels.*.*.message.traits[*]..[?(@.type)]',
'$.channels.*.*.message.headers',
'$.channels.*.*.message.headers..[?(@.type)]',
'$.channels.*.*.message.payload',
'$.channels.*.*.message.payload..[?(@.type)]',
'$.channels.*.parameters.*.schema[?(@.type)]',
'$.channels.*.parameters.*.schema..[?(@.type)]',
'$.components.schemas..[?(@.type)]',
Comment on lines +18 to +25
Copy link
Member

Choose a reason for hiding this comment

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

Please have in mind that you don't need to define the type field in the schema, so you will skip some schemas. I can accept it for now, but we should fix that.

Copy link
Member Author

@KhudaDad414 KhudaDad414 Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking about it and I couldn't find a perfect way to define a schema worth optimizing. If we set the bar too low like all schemas should be optimized if it is an object , then the library will extract an unnecessary amount of schemas.
like {type: string} should be ignored since by optimizing it, we make it complicated and hard to understand.

if we set the bar too high then it will skip some schemas.

what is the perfect balance here?
the best that I could come up with is:

a schema should be optimized if: 
* it has more than one non-object or non-array field
or
* it has at least one object or array field.

what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

For that delay 😅

it has at least one object or array field.

Yeah, it's good atm :) We should remember about that "restriction". Could you create issue for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

@magicmatatjahu Sorry, I don't get it, should I leave it as it is or implement the restriction that I have proposed? or remove that having a type field restriction entirely?

Copy link
Member

Choose a reason for hiding this comment

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

as you suggested :)

Copy link
Member

Choose a reason for hiding this comment

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

but, that PR is a refactor, so we won't release it. I can accept it and you will work on it in the next PR, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@magicmatatjahu that would be great. can you approve this PR then?

];

parameterPaths = ['$.channels.*.parameters.*', '$.components.parameters.*'];
messages = new Map<string, Message>();
schemas = new Map<string, Schema>();
parameters = new Map<string, ChannelParameter>();

constructor(private document: AsyncAPIDocument) {
this.scanChannels();
this.scanComponents();
}

private scanChannels(): void {
const channels = this.document.channels();
for (const channelName in channels) {
const path = `channels.${channelName}`;
const channel = this.document.channel(channelName);

if (channel.hasPublish()) {
const message = channel.publish().message();
const currentPath = `${path}.publish.message`;
this.scanMessage(currentPath, message);
this.messages.set(currentPath, message);
}

if (channel.hasSubscribe()) {
const message = channel.subscribe().message();
const currentPath = `${path}.subscribe.message`;
this.scanMessage(currentPath, message);
this.messages.set(currentPath, message);
}

if (channel.hasParameters()) {
const currentPath = `${path}.parameters`;
for (const [name, channelParameter] of Object.entries(channel.parameters())) {
this.parameters.set(`${currentPath}.${name}`, channelParameter);
}
}
}
}

private scanSchema(path: string, schema: Schema): void {
if (!schema) {return;}
this.schemas.set(path, schema);
const schemaProperties = schema.properties();
for (const [propertyName, propertySchema] of Object.entries(schemaProperties)) {
this.schemas.set(`${path}.properties.${propertyName}`, propertySchema);
}
}

private scanMessageTraits(path: string, traits: MessageTrait[]): void {
for (const [index, trait] of Object.entries(traits)) {
if (trait.headers()) {
this.scanSchema(`${path}[${index}].headers`, trait.headers());
}
}
this.messages = this.parseComponents(this.messagePaths);
this.schemas = this.parseComponents(this.schemaPaths);
this.parameters = this.parseComponents(this.parameterPaths);
}

private scanMessage(path: string, message: Message): void {
this.scanSchema(`${path}.payload`, message.payload());
this.scanSchema(`${path}.headers`, message.headers());
this.scanMessageTraits(`${path}.traits`, message.traits());
private toLodashPath(JSONPath: string) {
return JSONPath.replace(/'\]\['/g, '.')
.slice(3, -2)
.replace(/'\]/g, '')
.replace(/\['/g, '.');
}
KhudaDad414 marked this conversation as resolved.
Show resolved Hide resolved

private scanComponents(): void {
const components = this.document.components();
if (!components) {
return;
}

if (components.hasMessages()) {
for (const [messageName, message] of Object.entries(components.messages())) {
this.messages.set(`components.messages.${messageName}`, message);
}
}

if (components.hasSchemas()) {
for (const [schemaName, schema] of Object.entries(components.schemas())) {
this.schemas.set(`components.schemas.${schemaName}`, schema);
}
}

if (components.hasParameters()) {
for (const [parameterName, parameter] of Object.entries(components.parameters())) {
this.parameters.set(`components.parameters.${parameterName}`, parameter);
}
}
private parseComponents(paths: string[]): any {
return _.chain(paths)
.map((messagePath) => {
KhudaDad414 marked this conversation as resolved.
Show resolved Hide resolved
return JSONPath({
path: messagePath,
resultType: 'all',
json: this.document.json(),
});
})
.flatten()
.reduce((red, message) => {
return red.set(this.toLodashPath(message.path), message.value);
}, new Map())
.value();
KhudaDad414 marked this conversation as resolved.
Show resolved Hide resolved
}
}
2 changes: 1 addition & 1 deletion src/Optimizers/MoveToComponents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class MoveToComponents implements OptimizerInterface {
//check if the component already has a copy in components section of the specification. If it already has then we don't need to apply this optimization.
//It will be taken care of by ReuseComponents
if (this.doesHaveACopy(value1, components)) { continue; }
const componentName = value1.json().name || `${componentType}-${counter++}`;
const componentName = value1.name || `${componentType}-${counter++}`;
const target = `components.${componentType}s.${componentName}`;
elements.push({
path: key1,
Expand Down
4 changes: 2 additions & 2 deletions src/Utils/Helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ const compareComponents = (x: any, y: any): boolean => {
//Compares two components but also considers equality check. the referential equality check can be disabled by referentialEqualityCheck argument.
const isEqual = (component1: any, component2: any, referentialEqualityCheck: boolean): boolean => {
if (referentialEqualityCheck) {
return component1.json() === component2.json() || compareComponents(component1.json(), component2.json());
return component1 === component2 || compareComponents(component1, component2);
}
return component1.json() !== component2.json() && compareComponents(component1.json(), component2.json());
return component1 !== component2 && compareComponents(component1, component2);
};

const isInComponents = (path: string): boolean => {
Expand Down
12 changes: 6 additions & 6 deletions test/Utils/Helpers.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,28 @@ import _ from 'lodash';
import { compareComponents, isEqual, isInComponents, isInChannels, toJS } from '../../src/Utils';

describe('Helpers', () => {
const testObject1 = { json: jest.fn().mockReturnValueOnce({ streetlightId: { schema: { type: 'string', 'x-extension': 'different_value' } } }) };
const testObject1_copy = { json: jest.fn().mockReturnValueOnce({ streetlightId: { schema: { type: 'string', 'x-extension': 'value' } } }) };
const testObject2 = { json: jest.fn().mockReturnValueOnce({ streetlightId: { schema: { type: 'number' } } }) };
const testObject1 = { streetlightId: { schema: { type: 'string', 'x-extension': 'different_value' } } };
const testObject1_copy = { streetlightId: { schema: { type: 'string', 'x-extension': 'value' } } };
const testObject2 = { streetlightId: { schema: { type: 'number' } } };
const testObject2_reference = testObject2;

describe('compareComponents', () => {
test('should return true.', () => {
expect(compareComponents(testObject1.json(), testObject1_copy.json())).toEqual(true);
expect(compareComponents(testObject1, testObject1_copy)).toEqual(true);
});

test('should return false.', () => {
expect(compareComponents(testObject1.json(), testObject2.json())).toEqual(false);
expect(compareComponents(testObject1, testObject2)).toEqual(false);
});
});
describe('isEqual', () => {
test('should return true.', () => {
expect(isEqual(testObject2, testObject2_reference, true)).toEqual(true);
expect(isEqual(testObject1, testObject1_copy, true)).toEqual(true);
expect(isEqual(testObject1, testObject1_copy, false)).toEqual(true);
});

test('should return false.', () => {
expect(isEqual(testObject1, testObject1_copy, false)).toEqual(false);
expect(isEqual(testObject2, testObject2_reference, false)).toEqual(false);
});
});
Expand Down