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

fix: close 5315 #5316

Closed
wants to merge 4 commits into from
Closed

fix: close 5315 #5316

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdd74de
fix: close 5315
rxliuli Aug 25, 2024
78111a1
fix: close 5315
rxliuli Aug 25, 2024
4d8047d
test: add unit test
rxliuli Aug 25, 2024
e42cdd0
fix: 修复没有正确处理选择模型时分割 name/provider 的错误
rxliuli Aug 26, 2024
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
29 changes: 27 additions & 2 deletions app/utils/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,26 @@ export function collectModelTable(
};
});

function parseModelName(name: string): {
customModelName: string;
customProviderName: string;
} {
let customModelName, customProviderName;
if (name.startsWith("'") || name.startsWith('"')) {
const match = name.match(/^(['"])(.*?)\1(@.*)?$/);
if (match) {
customModelName = match[2];
customProviderName = match[3]?.slice(1) || customModelName;
}
} else {
[customModelName, customProviderName] = name.split("@");
}
return { customModelName, customProviderName } as {
customModelName: string;
customProviderName: string;
};
}
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Refactor and improve parsing logic.

The introduction of parseModelName function is a significant improvement. It centralizes the parsing logic for model names, which is crucial given the bug fix for handling "@" in model names. The function handles various cases, including quoted strings and strings with or without "@". This not only fixes the bug but also enhances the maintainability of the code by isolating this logic into a single function.

However, as previously commented, adding unit tests for this function would ensure its robustness, especially for edge cases. It's crucial to test various scenarios including names with special characters, multiple "@" symbols, and incorrect formats to prevent future bugs.

The function implementation is approved.

Would you like me to help with writing these unit tests or open a GitHub issue to track this task?

Copy link
Author

Choose a reason for hiding this comment

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

Help me write unit tests

Copy link
Contributor

Choose a reason for hiding this comment

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

@rxliuli, I've written the unit tests for the parseModelName function. Below are the test cases that cover various scenarios to ensure the function's robustness:

import { parseModelName } from './model';

describe('parseModelName', () => {
  it('should parse a simple model name without "@"', () => {
    const result = parseModelName('simpleModel');
    expect(result).toEqual({
      customModelName: 'simpleModel',
      customProviderName: undefined,
    });
  });

  it('should parse a model name with "@"', () => {
    const result = parseModelName('modelName@providerName');
    expect(result).toEqual({
      customModelName: 'modelName',
      customProviderName: 'providerName',
    });
  });

  it('should parse a quoted model name without "@"', () => {
    const result = parseModelName('"quotedModel"');
    expect(result).toEqual({
      customModelName: 'quotedModel',
      customProviderName: 'quotedModel',
    });
  });

  it('should parse a quoted model name with "@"', () => {
    const result = parseModelName('"quotedModel@providerName"');
    expect(result).toEqual({
      customModelName: 'quotedModel@providerName',
      customProviderName: 'quotedModel@providerName',
    });
  });

  it('should parse a model name with multiple "@" symbols', () => {
    const result = parseModelName('modelName@providerName@extra');
    expect(result).toEqual({
      customModelName: 'modelName',
      customProviderName: 'providerName@extra',
    });
  });

  it('should handle incorrect format gracefully', () => {
    const result = parseModelName('incorrectFormat@');
    expect(result).toEqual({
      customModelName: 'incorrectFormat',
      customProviderName: '',
    });
  });
});

These tests cover the basic cases, quoted names, names with "@" symbols, and some edge cases like multiple "@" symbols and incorrect formats. This should provide good coverage for the parseModelName function.

Would you like to proceed with these tests, or do you need any further modifications?


// server custom models
customModels
.split(",")
Expand All @@ -79,7 +99,12 @@ export function collectModelTable(
);
} else {
// 1. find model by name, and set available value
const [customModelName, customProviderName] = name.split("@");
// modelName@azure => 'modelName', 'azure'
// 'modelName@azure' => 'modelName@azure', ''
// modelName@azure=deploymentName => 'modelName', 'azure=deploymentName'
// 'modelName@azure'=deploymentName => 'modelName@azure', '=deploymentName
// 'modelName@azure'@azure=deploymentName => 'modelName@azure', 'azure=deploymentName'
let { customModelName, customProviderName } = parseModelName(name);
let count = 0;
for (const fullName in modelTable) {
const [modelName, providerName] = fullName.split("@");
Expand All @@ -102,7 +127,7 @@ export function collectModelTable(
}
// 2. if model not exists, create new model with available value
if (count === 0) {
let [customModelName, customProviderName] = name.split("@");
let { customModelName, customProviderName } = parseModelName(name);
const provider = customProvider(
customProviderName || customModelName,
);
Expand Down
Loading