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

Changing load/dump in source files #190641

Merged
merged 14 commits into from
Aug 21, 2024
Merged
Show file tree
Hide file tree
Changes from 9 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
2 changes: 1 addition & 1 deletion packages/kbn-es/src/utils/read_roles_from_resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/
import fs from 'fs';
import { extname } from 'path';
import { load as loadYaml } from 'js-yaml';
import { safeLoad as loadYaml } from 'js-yaml';

export const readRolesFromResource = (resourcePath: string) => {
if (!fs.existsSync(resourcePath) || extname(resourcePath) !== '.yml') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import chalk from 'chalk';
import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';
import { isPlainObjectType } from './is_plain_object_type';

/**
Expand All @@ -32,7 +32,7 @@ export function extractByJsonPointer(document: unknown, pointer: string): unknow
throw new Error(
`JSON Pointer ${chalk.bold(pointer)} resolution failure. Expected ${chalk.magenta(
path.join('/')
)} to be a plain object but it has type "${typeof target}" in \n\n${dump(document)}`
)} to be a plain object but it has type "${typeof target}" in \n\n${safeDump(document)}`
);
}

Expand Down Expand Up @@ -66,7 +66,7 @@ export function extractObjectByJsonPointer(
throw new Error(
`JSON Pointer resolution failure. Expected ${chalk.magenta(
pointer
)} to be a plain object in \n\n${dump(document)}`
)} to be a plain object in \n\n${safeDump(document)}`
);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-openapi-bundler/src/utils/read_document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import fs from 'fs/promises';
import { basename, extname } from 'path';
import { load } from 'js-yaml';
import { safeLoad } from 'js-yaml';
import chalk from 'chalk';
import { logger } from '../logger';
import { isPlainObjectType } from './is_plain_object_type';
Expand Down Expand Up @@ -45,7 +45,7 @@ async function readYamlFile(filePath: string): Promise<Record<string, unknown>>
// Typing load's result to Record<string, unknown> is optimistic as we can't be sure
// there is object inside a yaml file. We don't have this validation layer so far
// but using JSON Schemas here should mitigate this problem.
return load(await fs.readFile(filePath, { encoding: 'utf8' }));
return safeLoad(await fs.readFile(filePath, { encoding: 'utf8' }));
}

async function readJsonFile(filePath: string): Promise<Record<string, unknown>> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import fs from 'fs/promises';
import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';
import { dirname } from 'path';

export async function writeYamlDocument(filePath: string, document: unknown): Promise<void> {
Expand All @@ -31,13 +31,14 @@ function stringifyToYaml(document: unknown): string {

// Disable YAML Anchors https://yaml.org/spec/1.2.2/#3222-anchors-and-aliases
// It makes YAML much more human readable
return dump(clearedDocument, {
return safeDump(clearedDocument, {
noRefs: true,
sortKeys: sortYamlKeys,
skipInvalid: true, // Skip invalid types like `undefined`
Copy link
Contributor

@jeramysoucy jeramysoucy Aug 20, 2024

Choose a reason for hiding this comment

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

Adding skipInvalid: true allows the files to be written even if they contain invalid types (e.g. undefined). The result is that the invalid types will not be written to the file rather than the default behavior of throwing an exception.

Previous use of the dump function would write files containing invalid types. It looks like we relied on parsing the file contents to handling removing the invalid values, which may no longer be necessary.

});
} catch (e) {
// Try to stringify with YAML Anchors enabled
return dump(document, { noRefs: false, sortKeys: sortYamlKeys });
return safeDump(document, { noRefs: false, sortKeys: sortYamlKeys, skipInvalid: true }); // Skip invalid types like `undefined`
}
}

Expand Down
9 changes: 6 additions & 3 deletions packages/kbn-openapi-bundler/tests/bundler/bundle_specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
unlinkSync,
writeFileSync,
} from 'fs';
import { dump, load } from 'js-yaml';
import { safeDump, safeLoad } from 'js-yaml';
import { OpenAPIV3 } from 'openapi-types';
import { bundle, BundlerConfig } from '../../src/openapi_bundler';

Expand Down Expand Up @@ -56,7 +56,10 @@ function dumpSpecs(folderPath: string, oasSpecs: Record<string, OpenAPIV3.Docume
mkdirSync(folderPath, { recursive: true });

for (const [fileName, oasSpec] of Object.entries(oasSpecs)) {
writeFileSync(join(folderPath, `${fileName}.schema.yaml`), dump(oasSpec));
writeFileSync(
join(folderPath, `${fileName}.schema.yaml`),
safeDump(oasSpec, { skipInvalid: true }) // Skip invalid types like `undefined`
);
}
}

Expand All @@ -66,7 +69,7 @@ export function readBundledSpecs(folderPath: string): Record<string, OpenAPIV3.D
for (const fileName of readdirSync(folderPath)) {
const yaml = readFileSync(join(folderPath, fileName), { encoding: 'utf8' });

bundledSpecs[fileName] = load(yaml);
bundledSpecs[fileName] = safeLoad(yaml);
}

return bundledSpecs;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { readFileSync } from 'fs';
import { load } from 'js-yaml';
import { safeLoad } from 'js-yaml';
import { join } from 'path';
import { bundleFolder, readBundledSpecs } from './bundle_specs';

Expand All @@ -25,7 +25,7 @@ describe('OpenAPI Bundler - specs with multiple modifications', () => {

const [bundledSpec] = Object.values(readBundledSpecs(outputFolderPath));

const expected = load(
const expected = safeLoad(
readFileSync(join(folderToBundlePath, 'expected.yaml'), { encoding: 'utf8' })
);

Expand Down
5 changes: 3 additions & 2 deletions packages/kbn-openapi-bundler/tests/bundler/circular.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';
import { OpenAPIV3 } from 'openapi-types';
import { bundleSpecs } from './bundle_specs';
import { createOASDocument } from '../create_oas_document';
Expand Down Expand Up @@ -48,7 +48,8 @@ describe('OpenAPI Bundler - circular specs', () => {
})
);

expect(dump(bundledSpec.paths['/api/some_api']!.get!.responses['200'])).toMatchInlineSnapshot(`
expect(safeDump(bundledSpec.paths['/api/some_api']!.get!.responses['200']))
.toMatchInlineSnapshot(`
"content:
application/json:
schema: &ref_0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ describe('OpenAPI Bundler - remove custom x- props', () => {
expect(bundledSpec.paths['/api/some_api']!.post).not.toMatchObject({
'x-codegen-enabled': expect.anything(),
});
// As we have switched to using the js-yaml safeDump function, we are stripping
// out invalid types like 'undefined' when the spec is written to file.
Copy link
Contributor

Choose a reason for hiding this comment

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

To reviewers: does it make sense to refactor this test, or rather to implement unit tests for the write_yaml_document.ts utility? The end result is the same, hence this test passes, but the behavior at the utility level is different.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact write_yaml_document.ts contained a hack to remove undefined via JSON.parse(JSON.stringinfy()). I removed this hack so the result behavior stays the same.

The output of this utility is committed to the repo so we can immediately spot any inconsistencies. It also means that having a lot of unit tests to verify all aspects isn't strictly necessary.

expect(bundledSpec.paths['/api/some_api']!.put).not.toMatchObject({
'x-codegen-enabled': expect.anything(),
});
Expand Down
6 changes: 3 additions & 3 deletions packages/kbn-openapi-bundler/tests/merger/merge_specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
unlinkSync,
writeFileSync,
} from 'fs';
import { dump, load } from 'js-yaml';
import { safeDump, safeLoad } from 'js-yaml';
import { OpenAPIV3 } from 'openapi-types';
import { merge, MergerConfig } from '../../src/openapi_merger';

Expand Down Expand Up @@ -56,7 +56,7 @@ function dumpSpecs(folderPath: string, oasSpecs: Record<string, OpenAPIV3.Docume
mkdirSync(folderPath, { recursive: true });

for (const [fileName, oasSpec] of Object.entries(oasSpecs)) {
writeFileSync(join(folderPath, `${fileName}.schema.yaml`), dump(oasSpec));
writeFileSync(join(folderPath, `${fileName}.schema.yaml`), safeDump(oasSpec));
}
}

Expand All @@ -66,7 +66,7 @@ export function readMergedSpecs(folderPath: string): Record<string, OpenAPIV3.Do
for (const fileName of readdirSync(folderPath)) {
const yaml = readFileSync(join(folderPath, fileName), { encoding: 'utf8' });

mergedSpecs[fileName] = load(yaml);
mergedSpecs[fileName] = safeLoad(yaml);
}

return mergedSpecs;
Expand Down
2 changes: 1 addition & 1 deletion scripts/enabled_ftr_configs.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ var allManifestPaths = Object.values(manifestsSource).flat();

try {
for (var manifestRelPath of allManifestPaths) {
var manifest = yaml.load(fs.readFileSync(manifestRelPath, 'utf8'));
var manifest = yaml.safeLoad(fs.readFileSync(manifestRelPath, 'utf8'));
if (manifest.enabled) {
manifest.enabled.forEach(function (x) {
console.log(x);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import { readFileSync, writeFileSync } from 'fs';
import { resolve } from 'path';
import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';
import { Build, Config, mkdirp } from '../../lib';

export async function createOSPackageKibanaYML(config: Config, build: Build) {
Expand All @@ -24,7 +24,7 @@ export async function createOSPackageKibanaYML(config: Config, build: Build) {
[/#pid.file:.*/g, 'pid.file: /run/kibana/kibana.pid'],
[
/#logging.appenders.default:.*kibana\.log\n/gs,
dump({
safeDump({
logging: {
appenders: {
file: {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/packages/kbn-data-forge/src/lib/create_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { DEFAULTS } from '../constants';

export async function readConfig(filePath: string): Promise<PartialConfig> {
const data = await promises.readFile(filePath);
const decodedPartialConfig = PartialConfigRT.decode(yaml.load(data.toString()));
const decodedPartialConfig = PartialConfigRT.decode(yaml.safeLoad(data.toString()));
if (isLeft(decodedPartialConfig)) {
throw new Error(
`Could not validate config: ${PathReporter.report(decodedPartialConfig).join('\n')}`
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/cloud_defend/common/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export function getSelectorsAndResponsesFromYaml(configuration: string): {
let responses: Response[] = [];

try {
const result = yaml.load(configuration);
const result = yaml.safeLoad(configuration);

if (result) {
// iterate selector/response types
Expand Down Expand Up @@ -107,5 +107,5 @@ export function getYamlFromSelectorsAndResponses(selectors: Selector[], response
return current;
}, schema);

return yaml.dump(schema);
return yaml.safeDump(schema);
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('<ControlGeneralView />', () => {
const configuration = input?.vars?.configuration?.value;

try {
const json = yaml.load(configuration);
const json = yaml.safeLoad(configuration);

expect(json.file.selectors.length).toBe(getAllByTestId('cloud-defend-selector').length);
expect(json.file.responses.length).toBe(getAllByTestId('cloud-defend-file-response').length);
Expand All @@ -69,7 +69,7 @@ describe('<ControlGeneralView />', () => {
const configuration = input?.vars?.configuration?.value;

try {
const json = yaml.load(configuration);
const json = yaml.safeLoad(configuration);

expect(json.file.selectors.length).toBe(getAllByTestId('cloud-defend-selector').length);
} catch (err) {
Expand All @@ -91,7 +91,7 @@ describe('<ControlGeneralView />', () => {
const configuration = input?.vars?.configuration?.value;

try {
const json = yaml.load(configuration);
const json = yaml.safeLoad(configuration);

expect(json.file.responses.length).toBe(getAllByTestId('cloud-defend-file-response').length);
} catch (err) {
Expand All @@ -113,7 +113,7 @@ describe('<ControlGeneralView />', () => {
const configuration = input?.vars?.configuration?.value;

try {
const json = yaml.load(configuration);
const json = yaml.safeLoad(configuration);

expect(json.process.responses.length).toBe(
getAllByTestId('cloud-defend-process-response').length
Expand Down Expand Up @@ -166,7 +166,7 @@ describe('<ControlGeneralView />', () => {
const configuration = input?.vars?.configuration?.value;

try {
const json = yaml.load(configuration);
const json = yaml.safeLoad(configuration);

expect(json.file.responses[0].match).toHaveLength(1);
} catch (err) {
Expand Down Expand Up @@ -205,7 +205,7 @@ describe('<ControlGeneralView />', () => {
const configuration = input?.vars?.configuration?.value;

try {
const json = yaml.load(configuration);
const json = yaml.safeLoad(configuration);

expect(json.file.selectors).toHaveLength(4);
expect(json.file.selectors[3].name).toEqual(json.file.selectors[0].name + '1');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ function decodeDiscoveryRulesYaml(
defaultDiscoveryRules: IDiscoveryRule[] = []
): IDiscoveryRule[] {
try {
const parsedYaml: DiscoveryRulesParsedYaml = yaml.load(discoveryRulesYaml) ?? [];
const parsedYaml: DiscoveryRulesParsedYaml = yaml.safeLoad(discoveryRulesYaml) ?? [];

if (parsedYaml.length === 0) {
return defaultDiscoveryRules;
Expand All @@ -232,5 +232,5 @@ function encodeDiscoveryRulesYaml(discoveryRules: IDiscoveryRule[]): string {
[`${operation}-${type}`]: probe,
})
);
return yaml.dump(mappedDiscoveryRules);
return yaml.safeDump(mappedDiscoveryRules);
}
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ function ensureValidMultiText(textMultiValue: string[] | undefined) {

function escapeInvalidYamlString(yamlString: string) {
try {
yaml.load(yamlString);
yaml.safeLoad(yamlString);
} catch (error) {
if (error instanceof yaml.YAMLException) {
return `"${yamlString}"`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';
import { generateCustomLogsYml } from './generate_custom_logs_yml';

const baseMockConfig = {
Expand Down Expand Up @@ -46,7 +46,7 @@ describe('generateCustomLogsYml', () => {
it('should return a yml configuration with customConfigurations', () => {
const mockConfig = {
...baseMockConfig,
customConfigurations: dump({
customConfigurations: safeDump({
['agent.retry']: {
enabled: true,
retriesCount: 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { dump, load } from 'js-yaml';
import { safeDump, safeLoad } from 'js-yaml';

export const generateCustomLogsYml = ({
datasetName = '',
Expand All @@ -26,7 +26,7 @@ export const generateCustomLogsYml = ({
esHost: string[];
logfileId: string;
}) => {
const customConfigYaml = load(customConfigurations ?? '');
const customConfigYaml = safeLoad(customConfigurations ?? '');
const processors = [
{
add_fields: {
Expand All @@ -38,7 +38,7 @@ export const generateCustomLogsYml = ({
},
];

return dump({
return safeDump({
...{
outputs: {
default: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';

interface SystemLogsStream {
id: string;
Expand Down Expand Up @@ -36,7 +36,7 @@ export const generateSystemLogsYml = ({
esHost: string[];
uuid: string;
}) => {
return dump({
return safeDump({
outputs: {
default: {
type: 'elasticsearch',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
FleetUnauthorizedError,
type PackageClient,
} from '@kbn/fleet-plugin/server';
import { dump } from 'js-yaml';
import { safeDump } from 'js-yaml';
import { PackageDataStreamTypes } from '@kbn/fleet-plugin/common/types';
import { getObservabilityOnboardingFlow, saveObservabilityOnboardingFlow } from '../../lib/state';
import type { SavedObservabilityOnboardingFlow } from '../../saved_objects/observability_onboarding_status';
Expand Down Expand Up @@ -501,7 +501,7 @@ function parseIntegrationsTSV(tsv: string) {
}

const generateAgentConfig = ({ esHost, inputs = [] }: { esHost: string[]; inputs: unknown[] }) => {
return dump({
return safeDump({
outputs: {
default: {
type: 'elasticsearch',
Expand Down
Loading