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

feat: created specificationUrl class #110

Merged
merged 17 commits into from
Jan 4, 2022
Merged
Show file tree
Hide file tree
Changes from 12 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
34 changes: 28 additions & 6 deletions package-lock.json

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

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"@types/ws": "^8.2.0",
"chokidar": "^3.5.2",
"inquirer": "^8.2.0",
"node-fetch": "^2.0.0",
"open": "^8.4.0",
"chalk": "^4.1.0",
"indent-string": "^4.0.0",
Expand All @@ -42,6 +43,7 @@
"@types/lodash.template": "^4.4.4",
"@types/mocha": "^5.2.7",
"@types/node": "^10.17.60",
"@types/node-fetch": "^2.0.2",
"@types/serve-handler": "^6.1.1",
"@types/wrap-ansi": "^8.0.1",
"@typescript-eslint/eslint-plugin": "^4.28.4",
Expand Down
4 changes: 2 additions & 2 deletions src/commands/start/studio.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ export default class StartStudio extends Command {

async run() {
const { flags } = this.parse(StartStudio);
const filePath = flags.file || (await load()).getPath();
const filePath = flags.file || (await load()).getFilePath();
const port = flags.port;

startStudio(filePath, port);
startStudio(filePath as string, port);
}
}
16 changes: 11 additions & 5 deletions src/commands/validate.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {flags } from '@oclif/command';
import { flags } from '@oclif/command';
import * as parser from '@asyncapi/parser';
import Command from '../base';
import { ValidationError } from '../errors/validation-error';
Expand All @@ -13,14 +13,14 @@ export default class Validate extends Command {
}

static args = [
{ name: 'spec-file', description: 'spec path or context-name', required: false },
{ name: 'spec-file', description: 'spec path, context-name or spec url', required: false },
Souvikns marked this conversation as resolved.
Show resolved Hide resolved
]

async run() {
const { args } = this.parse(Validate);
const filePath = args['spec-file'];
let specFile;

try {
specFile = await load(filePath);
} catch (err) {
Expand All @@ -34,8 +34,14 @@ export default class Validate extends Command {
}
}
try {
await parser.parse(await specFile.read());
this.log(`File ${specFile.getPath()} successfully validated!`);
if (specFile.getFilePath()) {
await parser.parse(specFile.text());
this.log(`File ${specFile.getFilePath()} successfully validated!`);
}
if (specFile.getURLPath()) {
Souvikns marked this conversation as resolved.
Show resolved Hide resolved
await parser.parse(specFile.text());
this.log(`URL ${specFile.getURLPath()} successfully validated`);
}
} catch (error) {
throw new ValidationError({
type: 'parser-error',
Expand Down
7 changes: 7 additions & 0 deletions src/errors/specification-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,10 @@ export class SpecificationFileNotFound extends SpecificationFileError {
}
}
}

export class SpecificationURLNotFound extends SpecificationFileError {
constructor(URL: string) {
super();
this.message = `Unable to fetch specification file from url: ${URL}`;
}
}
70 changes: 62 additions & 8 deletions src/models/SpecificationFile.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { promises as fs } from 'fs';
import * as path from 'path';
import fetch from 'node-fetch';

import { loadContext } from './Context';
import { SpecificationFileNotFound } from '../errors/specification-file';
import { SpecificationFileNotFound, SpecificationURLNotFound } from '../errors/specification-file';

const { readFile, lstat } = fs;
const allowedFileNames: string[] = [
Expand All @@ -12,14 +13,52 @@ const allowedFileNames: string[] = [
];
const TYPE_CONTEXT_NAME = 'context-name';
const TYPE_FILE_PATH = 'file-path';
const TYPE_URL_PATH = 'url-path';

export class Specification {
Souvikns marked this conversation as resolved.
Show resolved Hide resolved
private readonly spec: string;
private readonly filePath?: string;
private readonly URLPath?: string;
constructor(spec: string, options?: { filepath?: string, URLPath?: string }) {
this.spec = spec;
this.filePath = options?.filepath;
this.URLPath = options?.URLPath;
}

text() {
return this.spec;
}

getFilePath() {
return this.filePath;
}

getURLPath() {
Copy link
Member

Choose a reason for hiding this comment

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

I think you shouldn't call this "URL path". That's a URL.

I mean, this is a URL: https://www.asynapi.com/blog.
And this is the URL path: /blog.

They're not the same and I think here you mean the full URL, not just the path part.

Why not getFileURL()?

Suggested change
getURLPath() {
getFileURL() {

If you change it, you'd have to change everything else following similar notation, like const TYPE_URL_PATH = 'url-path';

return this.URLPath;
}

static async fromFile(filepath: string) {
return new Specification(await readFile(filepath, { encoding: 'utf8' }), { filepath });
}

static async fromURL(URLpath: string) {
let response;
try {
response = await fetch(URLpath, { method: 'GET' });
} catch (error) {
throw new SpecificationURLNotFound(URLpath);
}
return new Specification(await response?.text() as string, { URLPath: URLpath });
}
}

export default class SpecificationFile {
private readonly pathToFile: string;

constructor(filePath: string) {
this.pathToFile = filePath;
}

getPath(): string {
return this.pathToFile;
}
Expand All @@ -29,22 +68,26 @@ export default class SpecificationFile {
}
}

export async function load(filePathOrContextName?: string): Promise<SpecificationFile> {
export async function load(filePathOrContextName?: string): Promise<Specification> {
if (filePathOrContextName) {
const type = await nameType(filePathOrContextName);
if (type === TYPE_CONTEXT_NAME) {
return loadFromContext(filePathOrContextName);
}
}

if (type === TYPE_URL_PATH) {
return Specification.fromURL(filePathOrContextName);
}
await fileExists(filePathOrContextName);
return new SpecificationFile(filePathOrContextName);
return Specification.fromFile(filePathOrContextName);
}

try {
return await loadFromContext();
} catch (e) {
const autoDetectedSpecFile = await detectSpecFile();
if (autoDetectedSpecFile) {
return new SpecificationFile(autoDetectedSpecFile);
return Specification.fromFile(autoDetectedSpecFile);
}
if (!filePathOrContextName || !autoDetectedSpecFile) {
throw e;
Expand All @@ -65,10 +108,21 @@ export async function nameType(name: string): Promise<string> {
}
return TYPE_CONTEXT_NAME;
} catch (e) {
if (await isURL(name)) {return TYPE_URL_PATH;}
return TYPE_CONTEXT_NAME;
}
}

export async function isURL(urlpath: string): Promise<boolean> {
try {
const url = new URL(urlpath);
if (url.protocol === 'http:' || url.protocol === 'https:') {return true;}
Souvikns marked this conversation as resolved.
Show resolved Hide resolved
} catch (error) {
return false;
}
return false;
}

export async function fileExists(name: string): Promise<boolean> {
try {
if ((await lstat(name)).isFile()) {
Expand All @@ -80,9 +134,9 @@ export async function fileExists(name: string): Promise<boolean> {
}
}

async function loadFromContext(contextName?: string): Promise<SpecificationFile> {
async function loadFromContext(contextName?: string): Promise<Specification> {
const context = await loadContext(contextName);
return new SpecificationFile(context);
return Specification.fromFile(context);
}

async function detectSpecFile(): Promise<string | undefined> {
Expand Down
10 changes: 10 additions & 0 deletions test/commands/validate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ describe('validate', () => {
expect(ctx.stderr).to.equals('ValidationError: There is no file or context with name "./test/not-found.yml".\n');
done();
});

test
.stderr()
.stdout()
.command(['validate', 'https://bit.ly/asyncapi'])
Copy link
Member

Choose a reason for hiding this comment

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

I see that @fmvilas already reviews, so from my point of view I only suggest to log a followup issue, something good for the first contribution, to make sure tests do not rely on external resources. Basically, external resources are risky to use in tests and can cause tests to be flaky, like randomly failing just because the external resource is not available. So follow up should describe that tests should actually start independent server hosting asyncapi file, like we do in parser -> https://github.com/asyncapi/parser-js/blob/master/package.json#L20

.it('works when url is passed', (ctx, done) => {
expect(ctx.stdout).to.equals('URL https://bit.ly/asyncapi successfully validated\n');
expect(ctx.stderr).to.equals('');
done();
});
});

describe('with context names', () => {
Expand Down