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

chore(Rest): Suggestion to Rest work #1

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

lordrip
Copy link

@lordrip lordrip commented Oct 3, 2024

Context

This PR adds a few suggestions to the existing KaotoIO#1524 PR.

It also fixes the problem of some typings not loading.

relates to: KaotoIO#1490

@@ -40,8 +40,5 @@
"husky": "^9.0.0",
"lint-staged": "^15.0.0"
},
"packageManager": "[email protected]",
"dependencies": {
"fs": "^0.0.1-security"
Copy link
Author

Choose a reason for hiding this comment

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

I thought this wasn't needed, so I removed it from the dependency list

@@ -115,6 +115,7 @@
"@types/lodash.memoize": "^4.1.9",
"@types/lodash.set": "^4.3.7",
"@types/node": "^20.0.0",
"@types/openapi-v3": "^3.0.0",
Copy link
Author

Choose a reason for hiding this comment

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

I added this type that resembles the official OpenApi 3.0 schema

[K in keyof Required<OpenApiPath>]: Required<OpenApiPath>[K] extends OpenApiOperation ? K : never;
}[keyof OpenApiPath];

const VALID_METHODS: OpenApiPathMethods[] = ['get', 'post', 'put', 'delete', 'head', 'patch'];
Copy link
Author

Choose a reason for hiding this comment

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

This is the list of methods that Rest supports

Comment on lines -111 to -113
for (const path in openApi.paths) {
for (const method in openApi.paths[path]) {
const operationId = openApi.paths[path][method]['operationId'];
Copy link
Author

Choose a reason for hiding this comment

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

While this worked, the OpenApi spec defines other fields that we might be iterating over if present:

export interface OpenApiPath {
    /**
     * Allows for an external definition of this path item. The referenced structure MUST be in the format of a Path Item Object.
     * If there are conflicts between the referenced definition and this Path Item's definition, the behavior is undefined.
     */
    $ref?: string;
    /** An optional, string summary, intended to apply to all operations in this path. */
    summary?: string;
    /** An optional, string description, intended to apply to all operations in this path. CommonMark syntax MAY be used for rich text representation. */
    description?: string;
    /** A definition of a GET operation on this path. */
    get?: OpenApiOperation;
    /** A definition of a PUT operation on this path. */
    put?: OpenApiOperation;
    /** A definition of a POST operation on this path. */
    post?: OpenApiOperation;
    /** A definition of a DELETE operation on this path. */
    delete?: OpenApiOperation;
    /** A definition of a OPTIONS operation on this path. */
    options?: OpenApiOperation;
    /** A definition of a HEAD operation on this path. */
    head?: OpenApiOperation;
    /** A definition of a PATCH operation on this path. */
    patch?: OpenApiOperation;
    /** A definition of a TRACE operation on this path. */
    trace?: OpenApiOperation;
    /** An alternative server array to service all operations in this path. */
    servers?: OpenApiServer[];
   ...
}

So better to iterate only on the intended methods, hence the need for the valid methods array above.

}
}
});

if (!restExists) {
const entity = new CamelRestVisualEntity(rest);
const entity = new CamelRestVisualEntity({ rest });
Copy link
Author

Choose a reason for hiding this comment

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

The constructor of the CamelRestVisualEntity class is:

constructor(public restDef: { rest: Rest }) {

but we were passing rest: Rest directly

Comment on lines +26 to +31
'get',
'post',
'put',
'delete',
'head',
'patch',
Copy link
Author

Choose a reason for hiding this comment

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

Here we're removing these properties from the form, since it would be better to configure them through the UI, like adding nodes and stuff.

'patch',
];

constructor(public restDef: { rest: Rest }) {
Copy link
Author

Choose a reason for hiding this comment

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

I renamed this rest to restDef so it's easier to reason about what is being edited.

@@ -66,31 +75,21 @@ export class CamelRestVisualEntity extends AbstractCamelVisualEntity<{ rest: Res
const schema = CamelCatalogService.getComponent(CatalogKind.Entity, 'rest');

return {
definition: Object.assign({}, this.rest),
Copy link
Author

Choose a reason for hiding this comment

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

This was preventing the configuration form from loading because we were assigning the restDef instead of the rest property.

}

getNodeInteraction(): NodeInteraction {
return {
canHavePreviousStep: false,
canHaveNextStep: false,
canHaveChildren: false,
canHaveSpecialChildren: true,
canHaveSpecialChildren: false,
Copy link
Author

Choose a reason for hiding this comment

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

Since we are not providing the appropriate get, post, etc... nodes, would it be better to don't show this interaction?

@@ -148,6 +147,6 @@ export class CamelRestVisualEntity extends AbstractCamelVisualEntity<{ rest: Res
}

protected getRootUri(): string | undefined {
return this.rest.rest;
Copy link
Author

Choose a reason for hiding this comment

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

getRootUri should return either string or undefined, here we were returning Rest

@jcordes73 jcordes73 merged commit 52e4b1b into jcordes73:issue_1490 Oct 4, 2024
@lordrip lordrip deleted the chore/suggestions-to-rest branch October 14, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants