В веб-сервер добавлена возможность работы с данными форм. #1476
Conversation
|
Warning Rate limit exceeded@akpaevj has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughThe changes introduced in this pull request consist of several new classes and modifications to existing classes within the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/OneScript.Web.Server/FormCollectionWrapper.cs (1)
30-36: Ensure correct return type inGetIndexedValuemethodIn the
GetIndexedValuemethod, while implicit conversion fromStringValuestoStringValuesWrapperis available, explicitly wrapping the result enhances clarity and consistency.Suggested change:
public override StringValuesWrapper GetIndexedValue(IValue index) { if (_items.TryGetValue(index.AsString(), out var result)) - return result; + return new StringValuesWrapper(result); else - return StringValues.Empty; + return new StringValuesWrapper(StringValues.Empty); }src/OneScript.Web.Server/HeaderDictionaryWrapper.cs (2)
27-292: Explicitly wrap header values inStringValuesWrapperThe properties return
StringValuesWrapperbut are currently returningStringValuesinstances. While implicit conversion exists, explicitly wrapping the values improves readability.Suggested change for each property:
[ContextProperty("Accept", CanWrite = false)] - public StringValuesWrapper Accept => _items.Accept; + public StringValuesWrapper Accept => new StringValuesWrapper(_items.Accept);
307-313: Ensure correct return type inGetIndexedValuemethodSimilar to earlier, explicitly wrapping the result enhances clarity.
Suggested change:
public override StringValuesWrapper GetIndexedValue(IValue index) { if (_items.TryGetValue(index.AsString(), out var result)) - return result; + return new StringValuesWrapper(result); else - return StringValues.Empty; + return new StringValuesWrapper(StringValues.Empty); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/OneScript.Web.Server/FormCollectionWrapper.cs(1 hunks)src/OneScript.Web.Server/FormFileCollectionWrapper.cs(1 hunks)src/OneScript.Web.Server/FormFileWrapper.cs(1 hunks)src/OneScript.Web.Server/HeaderDictionaryWrapper.cs(4 hunks)src/OneScript.Web.Server/HttpRequestWrapper.cs(4 hunks)src/OneScript.Web.Server/HttpResponseWrapper.cs(1 hunks)src/OneScript.Web.Server/StringValuesWrapper.cs(1 hunks)src/OneScript.Web.Server/WebSockets/WebSocketAcceptContextWrapper.cs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/OneScript.Web.Server/HttpResponseWrapper.cs
🔇 Additional comments (8)
src/OneScript.Web.Server/StringValuesWrapper.cs (1)
1-79: Overall implementation looks good
The StringValuesWrapper class correctly wraps StringValues and provides the necessary methods and properties for indexed access and enumeration.
src/OneScript.Web.Server/FormCollectionWrapper.cs (1)
1-79: Overall implementation is correct
The FormCollectionWrapper class effectively wraps IFormCollection, providing indexed access and enumeration of form data.
src/OneScript.Web.Server/HeaderDictionaryWrapper.cs (1)
Line range hint 1-365: Updated header handling is appropriate
The modifications to handle multiple header values enhance functionality and align with HTTP standards.
src/OneScript.Web.Server/FormFileCollectionWrapper.cs (1)
1-65: Implementation is correct and follows best practices
The FormFileCollectionWrapper class properly wraps IFormFileCollection and exposes necessary methods for file retrieval and enumeration.
src/OneScript.Web.Server/FormFileWrapper.cs (1)
1-47: Well-implemented wrapper for IFormFile
The FormFileWrapper class correctly exposes the properties and methods of IFormFile, facilitating file handling in the OneScript environment.
src/OneScript.Web.Server/WebSockets/WebSocketAcceptContextWrapper.cs (1)
22-22: LGTM! Modern C# syntax improvements
The changes utilize C# 9.0 features (target-typed new) and remove redundant casts, improving code readability while maintaining the same functionality.
Let's verify the C# language version setting:
Also applies to: 32-32
✅ Verification successful
Let me verify the specific project's language version. I'll search for the project file containing WebSocketAcceptContextWrapper.cs.
Target-typed new syntax is compatible with project settings
The project uses C# 10.0 (LangVersion 10.0), which fully supports the target-typed new syntax introduced in C# 9.0. The changes are safe and aligned with the project's language version settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check .csproj files for language version
fd -e csproj . -x cat {} | grep -i 'LangVersion\|TargetFramework'
Length of output: 2028
Script:
#!/bin/bash
# Find the specific .csproj file for the WebSockets component
fd "OneScript.Web.Server.csproj|WebSocketAcceptContextWrapper.cs" . --type f -x dirname {} | sort | uniq
Length of output: 173
Script:
#!/bin/bash
# Check the specific project file's settings
cat ./src/OneScript.Web.Server/OneScript.Web.Server.csproj | grep -i 'LangVersion\|TargetFramework'
Length of output: 198
src/OneScript.Web.Server/HttpRequestWrapper.cs (2)
44-44: LGTM! Consistent use of modern C# syntax
The changes to use target-typed new expressions improve code readability while maintaining functionality.
Also applies to: 71-71, 74-74
136-146: Add XML documentation for the Form property
Consider adding XML documentation to maintain consistency with other properties in the class. Include description, return value details, and any relevant remarks about form content type requirements.
Here's a suggested documentation block:
+ /// <summary>
+ /// Gets the form data from the request if available.
+ /// </summary>
+ /// <returns>
+ /// FormCollectionWrapper instance if the request has form content type;
+ /// otherwise, returns null.
+ /// </returns>
[ContextProperty("Форма", "Form", CanWrite = false)]
public IValue FormLet's verify the form handling implementation:
| public override IValue GetIndexedValue(IValue index) | ||
| { | ||
| var value = (int)index.AsNumber(); | ||
|
|
||
| return ValueFactory.Create(_value[value]); |
There was a problem hiding this comment.
Add index validation to prevent exceptions in GetIndexedValue
The GetIndexedValue method currently casts the index to an integer and accesses the _value array without validating the index range. This could lead to an IndexOutOfRangeException if an invalid index is provided.
Consider adding validation to ensure the index is within the valid range:
public override IValue GetIndexedValue(IValue index)
{
var idx = (int)index.AsNumber();
+ if (idx < 0 || idx >= _value.Count)
+ return BslNullValue.Instance; // Or throw an appropriate exception
return ValueFactory.Create(_value[idx]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public override IValue GetIndexedValue(IValue index) | |
| { | |
| var value = (int)index.AsNumber(); | |
| return ValueFactory.Create(_value[value]); | |
| public override IValue GetIndexedValue(IValue index) | |
| { | |
| var idx = (int)index.AsNumber(); | |
| if (idx < 0 || idx >= _value.Count) | |
| return BslNullValue.Instance; // Or throw an appropriate exception | |
| return ValueFactory.Create(_value[idx]); | |
| } |
|
Поправлю отступы и обновлю пр. Что-то непонятное |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/OneScript.Web.Server/FormCollectionWrapper.cs(1 hunks)src/OneScript.Web.Server/FormFileCollectionWrapper.cs(1 hunks)src/OneScript.Web.Server/FormFileWrapper.cs(1 hunks)src/OneScript.Web.Server/HeaderDictionaryWrapper.cs(4 hunks)src/OneScript.Web.Server/HttpRequestWrapper.cs(4 hunks)src/OneScript.Web.Server/StringValuesWrapper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/OneScript.Web.Server/FormFileWrapper.cs
- src/OneScript.Web.Server/FormFileCollectionWrapper.cs
- src/OneScript.Web.Server/FormCollectionWrapper.cs
🔇 Additional comments (3)
src/OneScript.Web.Server/StringValuesWrapper.cs (1)
31-36: Add index validation to prevent exceptions in GetIndexedValue
The GetIndexedValue method casts the index to an integer and accesses _value without validating the index range. This could lead to an IndexOutOfRangeException if an invalid index is provided.
Apply this diff to add index validation:
public override IValue GetIndexedValue(IValue index)
{
var value = (int)index.AsNumber();
+ if (value < 0 || value >= _value.Count)
+ return BslNullValue.Instance; // Or throw an appropriate exception
return ValueFactory.Create(_value[value]);
}src/OneScript.Web.Server/HttpRequestWrapper.cs (2)
144-144: Consider returning Undefined instead of Null
Returning BslUndefinedValue.Instance may be more appropriate than BslNullValue.Instance when the Form property is not available, aligning with the language's semantics for undefined values.
Apply this change:
else
- return BslNullValue.Instance;
+ return BslUndefinedValue.Instance;44-44: Verify compatibility of target-typed new expressions
The properties Body, Cookies, and Headers use target-typed new expressions, which require C# 9.0 or later. Ensure that the project is set to use a compatible language version to prevent compilation issues.
Run the following script to check the project's language version:
Also applies to: 71-71, 74-74
✅ Verification successful
Target-typed new expressions are compatible with project settings
The project OneScript.Web.Server explicitly sets LangVersion to 10.0, which fully supports target-typed new expressions (introduced in C# 9.0). The code is safe to use this feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the project is set to use C# 9.0 or later.
# Test: Inspect the .csproj files for the LangVersion property. Expect: LangVersion is 9.0 or higher.
fd --type f --extension csproj | xargs grep -E '<LangVersion>(9\.0|latest)</LangVersion>'
Length of output: 91
Script:
#!/bin/bash
# Let's first check if there are any .csproj files and their content
fd --type f --extension csproj -x cat {}
Length of output: 54355
| public override StringValuesWrapper GetIndexedValue(IValue index) | ||
| { | ||
| if (_items.TryGetValue(index.AsString(), out var result)) | ||
| return ValueFactory.Create(result); | ||
| return result; | ||
| else | ||
| return ValueFactory.Create(); | ||
| return StringValues.Empty; |
There was a problem hiding this comment.
Ensure consistent return types in GetIndexedValue
The method GetIndexedValue returns StringValuesWrapper, but it currently returns StringValues instances. While implicit conversions may handle this, it's clearer and safer to explicitly return StringValuesWrapper objects.
Apply this diff to explicitly convert the return values:
public override StringValuesWrapper GetIndexedValue(IValue index)
{
if (_items.TryGetValue(index.AsString(), out var result))
- return result;
+ return new StringValuesWrapper(result);
else
- return StringValues.Empty;
+ return new StringValuesWrapper(StringValues.Empty);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public override StringValuesWrapper GetIndexedValue(IValue index) | |
| { | |
| if (_items.TryGetValue(index.AsString(), out var result)) | |
| return ValueFactory.Create(result); | |
| return result; | |
| else | |
| return ValueFactory.Create(); | |
| return StringValues.Empty; | |
| public override StringValuesWrapper GetIndexedValue(IValue index) | |
| { | |
| if (_items.TryGetValue(index.AsString(), out var result)) | |
| return new StringValuesWrapper(result); | |
| else | |
| return new StringValuesWrapper(StringValues.Empty); |
|
@akpaevj LGTM. Вливать? |
|
Да |
Доработка по #1473. В веб-сервер добавлена возможность работы с данными форм в контексте запроса
Breaking changes: изменен тип значения у значений хедеров. Теперь это коллекция строк, а не примитивный тип
Summary by CodeRabbit
Release Notes
New Features
FormCollectionWrapperandFormFileCollectionWrapperclasses for enhanced handling of form data and files.FormFileWrapperto manage individual file uploads.StringValuesWrapperclass to facilitate handling multiple string values in HTTP headers.Enhancements
HttpRequestWrapperto include aFormproperty for easy access to form data.HeaderDictionaryWrapperto support multiple values.Bug Fixes
Refactor
newexpressions across various wrapper classes.HttpResponseWrapperandWebSocketAcceptContextWrapperfor clarity.