ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/vault
File: ui/app/components/totp-edit.js (JavaScript)

Code before:
    this.flashMessages.success('Successfully deleted key');
  }

  createKey = task(
    waitFor(async (event) => {
      event.preventDefault();
      const { isValid, state, invalidFormMessage } = this.args.model.validate();
      this.modelValidations = isValid ? null : state;
      this.invalidFormAlert = invalidFormMessage;

      if (!isValid) return;
      try {
        const allFields = [...this.keyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;
        if (generate && exported) this.hasGenerated = true;
        else this.reset();
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key');
    })
  );
}


Review comment (suggestion):
same comment here about model state, does this work? I wonder if this model has some logic handling higher up because accessing model state doesn't work in newer workflows. (But I know when we paired the other day, you were seeing errors in the form)

Code after:
  createKey = task(
    waitFor(async (event) => {
      event.preventDefault();
      const { isValid, state, invalidFormMessage } = this.args.model.validate();
      this.modelValidations = isValid ? null : state;
      this.invalidFormAlert = invalidFormMessage;

      if (!isValid) return;
      try {
        const allFields = [...this.defaultKeyFormFields, ...Object.values(this.groups).flat()];
        await this.args.model.save({
          adapterOptions: {
            keyFormFields: allFields,
          },
        });
        const { generate, exported } = this.args.model;

        if (generate && exported) {
          // stay in this template and show QR code returned from response
          this.hasGenerated = true;
        } else {
          // nothing is returned from response, transition to key details route
          this.transitionToRoute(SHOW_ROUTE, this.args.model.name);
        }
      } catch (err) {
        // err will display via model state
        return;
      }
      this.flashMessages.success('Successfully created key.');
    })
  );
}
Repository: hashicorp/vault
File: ui/app/models/totp-key.js (JavaScript)

Code before:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import Model, { attr } from '@ember-data/model';
// eslint-disable-next-line ember/no-computed-properties-in-native-classes
import { alias } from '@ember/object/computed';
import { withFormFields } from 'vault/decorators/model-form-fields';
import { withModelValidations } from 'vault/decorators/model-validations';
import { withExpandedAttributes } from 'vault/decorators/model-expanded-attributes';
import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
import { isPresent } from '@ember/utils';

const ALGORITHMS = ['SHA1', 'SHA256', 'SHA512'];
const DIGITS = [6, 8];
const SKEW = [0, 1];

const validations = {
  accountName: [
    {
      validator(model) {
        const { generate, accountName } = model;
        // this is required when generate is true
        return generate && !isPresent(accountName) ? false : true;
      },
      message: "Account name can't be blank when the key is generated by Vault.",
    },
    {
      type: 'containsWhiteSpace',
      message:
        "Account name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  issuer: [
    {
      validator(model) {
        const { generate, issuer } = model;
        // this is required when generate is true
        return generate && !isPresent(issuer) ? false : true;
      },

Review comment (suggestion):
Unless these are referenced in other places in this file, they can be use directly as the values for the `possibleValues` keys below

Code after:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import Model, { attr } from '@ember-data/model';
import { withFormFields } from 'vault/decorators/model-form-fields';
import { withModelValidations } from 'vault/decorators/model-validations';
import { withExpandedAttributes } from 'vault/decorators/model-expanded-attributes';
import lazyCapabilities, { apiPath } from 'vault/macros/lazy-capabilities';
import { isPresent } from '@ember/utils';

const validations = {
  accountName: [
    {
      validator(model) {
        const { generate, accountName } = model;
        // this is required when generate is true
        return generate && !isPresent(accountName) ? false : true;
      },
      message: "Account name can't be blank when the key is generated by Vault.",
    },
    {
      type: 'containsWhiteSpace',
      message:
        "Account name contains whitespace. If this is desired, you'll need to encode it with %20 in API requests.",
      level: 'warn',
    },
  ],
  issuer: [
    {
      validator(model) {
        const { generate, issuer } = model;
        // this is required when generate is true
        return generate && !isPresent(issuer) ? false : true;
      },
      message: "Issuer can't be blank when when the key is generated by Vault.",
    },
  ],
  key: [
    {
      validator(model) {
Repository: hashicorp/vault
File: command/server.go (Go)

Code before:
		configErrors = append(configErrors, current.Validate(path)...)

		if config == nil {
			config = current
		} else {
			config = config.Merge(current)
		}
	}

	if config != nil && config.Entropy != nil && config.Entropy.Mode == configutil.EntropyAugmentation && constants.IsFIPS() {
		c.UI.Warn("WARNING: Entropy Augmentation is not supported in FIPS 140-2 Inside mode; disabling from server configuration!\n")
		config.Entropy = nil
	}

	entCheckRequestLimiter(c, config)

	return config, configErrors, nil
}

func (c *ServerCommand) runRecoveryMode() int {
	config, configErrors, err := c.parseConfig()
	if err != nil {
		c.UI.Error(err.Error())
		return 1
	}

	// Ensure at least one config was found.
	if config == nil {
		c.UI.Output(wrapAtLength(
			"No configuration files found. Please provide configurations with the " +
				"-config flag. If you are supplying the path to a directory, please " +
				"ensure the directory contains files with the .hcl or .json " +
				"extension."))
		return 1
	}

	// Update the 'log' related aspects of shared config based on config/env var/cli
	c.flags.applyLogConfigOverrides(config.SharedConfig)
	l, err := c.configureLogging(config)
	if err != nil {
		c.UI.Error(err.Error())
		return 1
	}
	c.logger = l
	c.allLoggers = append(c.allLoggers, l)

	// reporting Errors found in the config
	for _, cErr := range configErrors {
		c.logger.Warn(cErr.String())
	}


Review comment (suggestion):
This check is because in our Docker cluster tests, `FoundKeys` are not being added to the `SharedConfig`.

Code after:
		configErrors = append(configErrors, current.Validate(path)...)

		if config == nil {
			config = current
		} else {
			config = config.Merge(current)
		}
	}

	if config != nil && config.Entropy != nil && config.Entropy.Mode == configutil.EntropyAugmentation && constants.IsFIPS() {
		c.UI.Warn("WARNING: Entropy Augmentation is not supported in FIPS 140-2 Inside mode; disabling from server configuration!\n")
		config.Entropy = nil
	}
	entCheckRequestLimiter(c, config)

	return config, configErrors, nil
}

func (c *ServerCommand) runRecoveryMode() int {
	config, configErrors, err := c.parseConfig()
	if err != nil {
		c.UI.Error(err.Error())
		return 1
	}

	// Ensure at least one config was found.
	if config == nil {
		c.UI.Output(wrapAtLength(
			"No configuration files found. Please provide configurations with the " +
				"-config flag. If you are supplying the path to a directory, please " +
				"ensure the directory contains files with the .hcl or .json " +
				"extension."))
		return 1
	}

	// Update the 'log' related aspects of shared config based on config/env var/cli
	c.flags.applyLogConfigOverrides(config.SharedConfig)
	l, err := c.configureLogging(config)
	if err != nil {
		c.UI.Error(err.Error())
		return 1
	}
	c.logger = l
	c.allLoggers = append(c.allLoggers, l)

	// reporting Errors found in the config
	for _, cErr := range configErrors {
		c.logger.Warn(cErr.String())
	}

	// Ensure logging is flushed if initialization fails
Repository: hashicorp/vault
File: ui/lib/core/addon/components/form-field.js (JavaScript)

Code before:
        return false;
      }
    }
  }

  get hasRadioSubText() {
    // for 'radio' editType, check to see if every of the possibleValues has a subText and label
    return this.args?.attr?.options?.possibleValues?.any((v) => v.subText);
  }

  get hideLabel() {
    const { type, options } = this.args.attr;
    if (type === 'boolean' || type === 'object' || options?.isSectionHeader) {
      return true;
    }
    // falsey values render a <FormFieldLabel>
    return this.shouldHideLabel.includes(options?.editType);
  }

  get disabled() {
    return this.args.disabled || false;
  }

  get showHelpText() {
    return this.args.showHelpText === false ? false : true;
  }

  // used in the label element next to the form element
  get labelString() {
    const label = this.args.attr.options?.label || '';
    if (label) {
      return label;
    }
    if (this.args.attr.name) {
      return capitalize([humanize([dasherize([this.args.attr.name])])]);
    }
    return '';
  }
  // both the path to mutate on the model, and the path to read the value from
  get valuePath() {
    return this.args.attr.options?.fieldValue || this.args.attr.name;
  }
  get isReadOnly() {
    const readonly = this.args.attr.options?.readOnly || false;
    return readonly && this.args.mode === 'edit';
  }
  get validationError() {
    const validations = this.args.modelValidations || {};
    const state = validations[this.valuePath];
    return state && !state.isValid ? state.errors.join(' ') : null;
  }

Review comment (suggestion):
I find this reads a little more naturally like this:
```suggestion
    if (this.args.showHelpText !== false && helpText) {
```

Code after:
        return false;
      }
    }
  }

  get hasRadioSubText() {
    // for 'radio' editType, check to see if every of the possibleValues has a subText and label
    return this.args?.attr?.options?.possibleValues?.any((v) => v.subText);
  }

  get hideLabel() {
    const { type, options } = this.args.attr;
    if (type === 'boolean' || type === 'object' || options?.isSectionHeader) {
      return true;
    }
    // falsey values render a <FormFieldLabel>
    return this.shouldHideLabel.includes(options?.editType);
  }

  get disabled() {
    return this.args.disabled || false;
  }

  get helpTextString() {
    const helpText = this.args.attr?.options?.helpText;
    if (this.args.showHelpText !== false && helpText) {
      return helpText;
    }
    return '';
  }

  // used in the label element next to the form element
  get labelString() {
    const label = this.args.attr.options?.label || '';
    if (label) {
      return label;
    }
    if (this.args.attr.name) {
      return capitalize([humanize([dasherize([this.args.attr.name])])]);
    }
    return '';
  }
  // both the path to mutate on the model, and the path to read the value from
  get valuePath() {
    return this.args.attr.options?.fieldValue || this.args.attr.name;
  }
  get isReadOnly() {
    const readonly = this.args.attr.options?.readOnly || false;
    return readonly && this.args.mode === 'edit';
  }
  get validationError() {
Repository: hashicorp/vault
File: ui/app/utils/api-error-message.ts (TypeScript)

Code before:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import { ErrorContext, ApiError } from 'vault/api';

// accepts an error and returns error.errors joined with a comma, error.message or a fallback message
export default async function (error: unknown, fallbackMessage = 'An error occurred, please try again') {
  const messageOrFallback = (message?: string) => message || fallbackMessage;

  if ((error as ErrorContext).response instanceof Response) {
    const apiError: ApiError = await (error as ErrorContext).response?.json();

    if (apiError.errors && typeof apiError.errors[0] === 'string') {
      return apiError.errors.join(', ');
    }
    return messageOrFallback(apiError.message);
  }

  return messageOrFallback((error as Error)?.message);
}


Review comment (suggestion):
This was derived from the existing `error-message` util. The functionality is the same but handles calling the `json` method on the response.

Code after:
 * SPDX-License-Identifier: BUSL-1.1
 */

/**
 * this util was derived from error-message and updated to handle the error context returned from the api service
 * once Ember Data is fully removed, the error-message util will also be removed
 * for all requests made with the api service, use this util to display error messages from server
 */

import { ErrorContext, ApiError } from 'vault/api';

// accepts an error and returns error.errors joined with a comma, error.message or a fallback message
export default async function (error: unknown, fallbackMessage = 'An error occurred, please try again') {
  const messageOrFallback = (message?: string) => message || fallbackMessage;

  if ((error as ErrorContext).response instanceof Response) {
    const apiError: ApiError = await (error as ErrorContext).response?.json();

    if (apiError.errors && typeof apiError.errors[0] === 'string') {
      return apiError.errors.join(', ');
    }
    return messageOrFallback(apiError.message);
  }

  return messageOrFallback((error as Error)?.message);
}
Repository: hashicorp/vault
File: ui/api-client/src/runtime.ts (TypeScript)

Code before:
  lease_id: string;
  mount_type: string;
  renewable: boolean;
  request_id: string;
  warnings: Array<string> | null;
  wrap_info: {
    accessor: string;
    creation_path: string;
    creation_time: string;
    wrapped_accessor: string;
    token: string;
    ttl: number;
  } | null;
}

export interface ResponseTransformer<T> {
    (json: any): T;
}

export class JSONApiResponse<T> {
    constructor(public raw: Response, private transformer: ResponseTransformer<T> = (jsonValue: any) => jsonValue) {}

    async value(): Promise<T> {
        return this.transformer(await this.raw.json());
    }
}

export class VoidApiResponse {
    constructor(public raw: Response) {}
    async value(): Promise<VoidResponse> {
        return await this.raw.json();
    }
}

export class BlobApiResponse {
    constructor(public raw: Response) {}

    async value(): Promise<Blob> {
        return await this.raw.blob();
    };
}

export class TextApiResponse {
    constructor(public raw: Response) {}

    async value(): Promise<string> {
        return await this.raw.text();
    };
}


Review comment (suggestion):
This is the same thing the middleware was doing but for all responses. Letting the client handle it means this will only be used for responses that are `JSONApiResponse` type and not for `VoidResponse`.

Code after:
  lease_id: string;
  mount_type: string;
  renewable: boolean;
  request_id: string;
  warnings: Array<string> | null;
  wrap_info: {
    accessor: string;
    creation_path: string;
    creation_time: string;
    wrapped_accessor: string;
    token: string;
    ttl: number;
  } | null;
}

export interface ResponseTransformer<T> {
    (json: any): T;
}

export class JSONApiResponse<T> {
    constructor(public raw: Response, private transformer: ResponseTransformer<T> = (jsonValue: any) => jsonValue) {}

    async value(): Promise<T> {
        const response = await this.raw.json();
        return this.transformer(response.data);
    }
}

export class VoidApiResponse {
    constructor(public raw: Response) {}
    async value(): Promise<VoidResponse> {
        return await this.raw.json();
    }
}

export class BlobApiResponse {
    constructor(public raw: Response) {}

    async value(): Promise<Blob> {
        return await this.raw.blob();
    };
}

export class TextApiResponse {
    constructor(public raw: Response) {}

    async value(): Promise<string> {
        return await this.raw.text();
    };
}
Repository: hashicorp/vault
File: ui/app/components/control-group-success.js (JavaScript)

Code before:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import { service } from '@ember/service';
import Component from '@ember/component';
import { task } from 'ember-concurrency';

export default Component.extend({
  router: service(),
  controlGroup: service(),
  store: service(),

  // public attrs
  model: null,
  controlGroupResponse: null,

  //internal state
  error: null,
  unwrapData: null,

  unwrap: task(function* (token) {
    const adapter = this.store.adapterFor('tools');
    this.set('error', null);
    try {
      const response = yield adapter.toolAction('unwrap', null, { clientToken: token });
      this.set('unwrapData', response.auth || response.data);
      this.controlGroup.deleteControlGroupToken(this.model.id);
    } catch (e) {
      this.set('error', `Token unwrap failed: ${e.errors[0]}`);
    }
  }).drop(),

  markAndNavigate: task(function* () {
    this.controlGroup.markTokenForUnwrap(this.model.id);

Review comment (suggestion):
I started glimmerizing this component and converting it to `ts` but it lead to a multitude of Ember Data models that needed to be typed which just didn't seem worth the time.

Code after:
/**
 * Copyright (c) HashiCorp, Inc.
 * SPDX-License-Identifier: BUSL-1.1
 */

import { service } from '@ember/service';
import Component from '@ember/component';
import { task } from 'ember-concurrency';
import apiErrorMessage from 'vault/utils/api-error-message';

export default Component.extend({
  router: service(),
  controlGroup: service(),
  api: service(),

  // public attrs
  model: null,
  controlGroupResponse: null,

  //internal state
  error: null,
  unwrapData: null,

  unwrap: task(function* (token) {
    this.set('error', null);
    try {
      const response = yield this.api.sys.unwrap({}, this.api.buildHeaders({ token }));
      this.set('unwrapData', response.auth || response.data);
      this.controlGroup.deleteControlGroupToken(this.model.id);
    } catch (e) {
      const error = yield apiErrorMessage(e);
      this.error = `Token unwrap failed: ${error}`;
    }
  }).drop(),

  markAndNavigate: task(function* () {
Repository: hashicorp/vault
File: ui/api-client/dist/esm/runtime.d.ts (TypeScript)

Code before:
}
export interface ResponseContext {
    fetch: FetchAPI;
    url: string;
    init: RequestInit;
    response: Response;
}
export interface ErrorContext {
    fetch: FetchAPI;
    url: string;
    init: RequestInit;
    error: unknown;
    response?: Response;
}
export interface Middleware {
    pre?(context: RequestContext): Promise<FetchParams | void>;
    post?(context: ResponseContext): Promise<Response | void>;
    onError?(context: ErrorContext): Promise<Response | void>;
}
export interface ApiResponse<T> {
    raw: Response;
    value(): Promise<T>;
}
export interface VoidResponse {
    auth: null;
    data: null;
    lease_duration: number;
    lease_id: string;
    mount_type: string;
    renewable: boolean;
    request_id: string;
    warnings: Array<string> | null;
    wrap_info: {
        accessor: string;
        creation_path: string;
        creation_time: string;
        wrapped_accessor: string;
        token: string;
        ttl: number;
    } | null;
}
export interface ResponseTransformer<T> {
    (json: any): T;
}
export declare class JSONApiResponse<T> {
    raw: Response;
    private transformer;
    constructor(raw: Response, transformer?: ResponseTransformer<T>);
    value(): Promise<T>;
}
export declare class VoidApiResponse {

Review comment (suggestion):
Something is off here. I updated this in the generator template but maybe didn't push the commit. Let me check on that but the type should be `unknown`.

Code after:
}
export interface ResponseContext {
    fetch: FetchAPI;
    url: string;
    init: RequestInit;
    response: Response;
}
export interface ErrorContext {
    fetch: FetchAPI;
    url: string;
    init: RequestInit;
    error: unknown;
    response?: Response;
}
export interface Middleware {
    pre?(context: RequestContext): Promise<FetchParams | void>;
    post?(context: ResponseContext): Promise<Response | void>;
    onError?(context: ErrorContext): Promise<Response | void>;
}
export interface ApiResponse<T> {
    raw: Response;
    value(): Promise<T>;
}
export interface VoidResponse {
    auth: unknown;
    data: unknown;
    lease_duration: number;
    lease_id: string;
    mount_type: string;
    renewable: boolean;
    request_id: string;
    warnings: Array<string> | null;
    wrap_info: {
        accessor: string;
        creation_path: string;
        creation_time: string;
        wrapped_accessor: string;
        token: string;
        ttl: number;
    } | null;
}
export interface ResponseTransformer<T> {
    (json: any): T;
}
export declare class JSONApiResponse<T> {
    raw: Response;
    private transformer;
    constructor(raw: Response, transformer?: ResponseTransformer<T>);
    value(): Promise<T>;
}
export declare class VoidApiResponse {
Repository: hashicorp/vault
File: plugins/database/mongodb/mongodb_test.go (Go)

Code before:
			actual, err := c.getTLSAuth()
			if test.expectErr && err == nil {
				t.Fatalf("err expected, got nil")
			}
			if !test.expectErr && err != nil {
				t.Fatalf("no error expected, got: %s", err)
			}
			assertDeepEqual(t, test.expectOpts, actual)
		})
	}
}

func appendToCertPool(t *testing.T, pool *x509.CertPool, caPem []byte) *x509.CertPool {
	t.Helper()

	ok := pool.AppendCertsFromPEM(caPem)
	if !ok {
		t.Fatalf("Unable to append cert to cert pool")
	}
	return pool
}

var cmpClientOptionsOpts = cmp.Options{
	cmpopts.IgnoreTypes(http.Transport{}),

	cmp.AllowUnexported(options.ClientOptions{}),

	cmp.AllowUnexported(tls.Config{}),
	cmpopts.IgnoreTypes(sync.Mutex{}, sync.RWMutex{}),

	// 'lazyCerts' has a func field which can't be compared.
	cmpopts.IgnoreFields(x509.CertPool{}, "lazyCerts"),
	cmp.AllowUnexported(x509.CertPool{}),
}

// Need a special comparison for ClientOptions because reflect.DeepEquals won't work in Go 1.16.
// See: https://github.com/golang/go/issues/45891
func assertDeepEqual(t *testing.T, a, b *options.ClientOptions) {
	t.Helper()

	if diff := cmp.Diff(a, b, cmpClientOptionsOpts); diff != "" {
		t.Fatalf("assertion failed: values are not equal\n--- expected\n+++ actual\n%v", diff)
	}
}

func createDBUser(t testing.TB, connURL, db, username, password string) {
	t.Helper()

	ctx, _ := context.WithTimeout(context.Background(), 10*time.Second)
	client, err := mongo.Connect(ctx, options.Client().ApplyURI(connURL))
	if err != nil {

Review comment (suggestion):
since you have this testCase structure, I wonder if we should make this a table driven test. Even if there is only one testcase.

Code after:
					&tls.Config{
						Certificates: []tls.Certificate{cert.TLSCert},
					},
				).
				SetAuth(options.Credential{
					AuthMechanism: "SCRAM-SHA-256",
					Username:      "unittest",
					Password:      "unittest",
				}),
			expectErr: false,
		},
	}

	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			c := new()
			c.Username = test.username
			c.Password = test.password
			c.TLSCAData = test.tlsCAData
			c.TLSCertificateKeyData = test.tlsKeyData

			actual, err := c.getTLSAuth()
			if test.expectErr && err == nil {
				t.Fatalf("err expected, got nil")
			}
			if !test.expectErr && err != nil {
				t.Fatalf("no error expected, got: %s", err)
			}
			assertDeepEqual(t, test.expectOpts, actual)
		})
	}
}

func appendToCertPool(t *testing.T, pool *x509.CertPool, caPem []byte) *x509.CertPool {
	t.Helper()

	ok := pool.AppendCertsFromPEM(caPem)
	if !ok {
		t.Fatalf("Unable to append cert to cert pool")
	}
	return pool
}

var cmpClientOptionsOpts = cmp.Options{
	cmpopts.IgnoreTypes(http.Transport{}),

	cmp.AllowUnexported(options.ClientOptions{}),

	cmp.AllowUnexported(tls.Config{}),
	cmpopts.IgnoreTypes(sync.Mutex{}, sync.RWMutex{}),