Repository navigation
feat(content-type): implement content type handling - #4
mwillbanks wants to merge 3 commits into
Conversation
WalkthroughAdds request body base64 decoding, multipart and urlencoded form parsing with file-backed uploads, header normalization and URI construction; introduces a parser library, tests, a Docker runtime image for Go+PHP AWS Lambda, small main refactor extracting environment setup, and .gitignore/.rr.yaml updates. Changes
Sequence Diagram(s)sequenceDiagram
actor API as API Gateway
participant Handler as Plugin Handler
participant Parser as Request Parser
participant FS as File System
participant Proto as Proto Request
API->>Handler: APIGatewayV2HTTPRequest (may include base64 body)
Handler->>Handler: decode base64 if flagged
Handler->>Parser: transformBody(body, content-type)
alt multipart/form-data
Parser->>FS: create temp file(s) for uploads
FS-->>Parser: temp file metadata
Parser-->>Handler: JSON fields + Uploads object
else application/x-www-form-urlencoded
Parser-->>Handler: JSON fields (no uploads)
else other
Parser-->>Handler: passthrough body
end
Handler->>Handler: normalizeHeaders()
Handler->>Handler: buildURI(path, rawQuery)
Handler->>Proto: populate request (body, headers, URI, uploads)
Proto-->>Handler: proto response context
Handler-->>API: API Gateway response (status, headers, body)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 golangci-lint (2.5.0)request_parser.go[error] 121-121: net/http.NewRequest must not be called. use net/http.NewRequestWithContext (noctx) [error] 143-143: net/http.NewRequest must not be called. use net/http.NewRequestWithContext (noctx) 🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin.go (1)
253-276: Handle url.QueryUnescape error on line 268.Line 268 silently ignores the error from
url.QueryUnescape(v.Value)withdecoded, _ := url.QueryUnescape(v.Value). This can mask malformed percent-encoding in cookie values. Check the error and either reject the cookie or log the issue.Note:
http.ParseCookiedoes return a slice of*http.Cookie, so the iteration pattern is correct.
🧹 Nitpick comments (13)
main.go (1)
82-85: Consider handling or logging os.Setenv errors.While
os.Setenvrarely fails in practice, silently ignoring errors could mask critical initialization issues, especially since this environment setup is essential for the Lambda runtime.Apply this diff to log errors:
func configureEnvironment() { - _ = os.Setenv("PATH", os.Getenv("PATH")+":"+os.Getenv("LAMBDA_TASK_ROOT")) - _ = os.Setenv("LD_LIBRARY_PATH", "./lib:/lib64:/usr/lib64") + if err := os.Setenv("PATH", os.Getenv("PATH")+":"+os.Getenv("LAMBDA_TASK_ROOT")); err != nil { + log.Printf("Warning: failed to set PATH: %v", err) + } + if err := os.Setenv("LD_LIBRARY_PATH", "./lib:/lib64:/usr/lib64"); err != nil { + log.Printf("Warning: failed to set LD_LIBRARY_PATH: %v", err) + } }php-conf/10-app.ini (1)
9-9: Consider Lambda timeout interaction.Setting
max_execution_time = 0disables PHP's execution time limit. While Lambda enforces its own timeout, this setting means PHP won't fail gracefully before Lambda terminates. Consider setting this to slightly less than your Lambda timeout to allow proper error handling.symfony/assets/controllers/csrf_protection_controller.js (1)
37-37: Consider removing themsCryptofallback.The
window.msCryptofallback was for Internet Explorer 11, which is no longer supported by Microsoft. Since this project targets modern browsers (evidenced by the use of Turbo/Hotwire), you can simplify to justwindow.crypto.- csrfField.defaultValue = csrfToken = btoa(String.fromCharCode.apply(null, (window.crypto || window.msCrypto).getRandomValues(new Uint8Array(18)))); + csrfField.defaultValue = csrfToken = btoa(String.fromCharCode.apply(null, window.crypto.getRandomValues(new Uint8Array(18))));plugin_test.go (2)
54-55: Consider checking proto unmarshal errors in tests.Ignoring the unmarshal error could mask test setup issues. While this is test code, failed unmarshalling would cause subsequent assertions to fail with confusing errors.
req := &httpV1proto.Request{} - _ = proto.Unmarshal(pld.Context, req) + if err := proto.Unmarshal(pld.Context, req); err != nil { + // Log or handle - helps debug test failures + }
362-366: Simplify nil check beforelen().The nil check is redundant since
len()on a nil map returns 0.if len(tt.cookies) > 0 { - if gotReq.Cookies == nil || len(gotReq.Cookies) == 0 { + if len(gotReq.Cookies) == 0 { t.Fatalf("expected cookies to be parsed") } }request_parser.go (3)
120-124: Usehttp.NewRequestWithContextfor proper context propagation.Per static analysis,
http.NewRequestshould be replaced withhttp.NewRequestWithContextto support request cancellation and timeouts.-func parseURLEncoded(body []byte, headers map[string]string) ([]byte, error) { - req, err := http.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) +func parseURLEncoded(ctx context.Context, body []byte, headers map[string]string) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/", bytes.NewReader(body))The same change should be applied to
parseMultipartat line 143.
98-103:exists()may returntruefor permission errors.
os.Statcan fail for reasons other than the file not existing (e.g., permission denied). In those cases, the function returnstrue, which could causeos.Removeto be called unnecessarily.func exists(path string) bool { - if _, err := os.Stat(path); os.IsNotExist(err) { - return false - } - return true + _, err := os.Stat(path) + return err == nil }
160-162: Silently ignoringf.Open()errors may obscure upload failures.While the
FileUpload.Errorfield captures failure reasons, consider logging or aggregating errors to aid debugging when uploads fail.request_parser_test.go (2)
11-30: Consider subtests for clearer classification failuresTable-driven test is fine, but wrapping each case in
t.Run(tt.name, func(t *testing.T) { ... })would isolate failures and show the failing case ingo testoutput more clearly.
62-129: Multipart test nicely validates uploads and cleanup; consider guaranteed cleanup on failureThe test thoroughly checks fields, upload metadata, and
uploads.Clear()removing the temp file. To avoid leaking temp files when an earlier assertion fails, you could add at.Cleanup(uploads.Clear)(ordefer uploads.Clear()) right afterparseMultipartreturns, while still keeping the explicit Clear/assertion at the end.symfony/tests/bootstrap.php (1)
11-13: Guard against undefinedAPP_DEBUGin bootstrapIf
APP_DEBUGisn’t set in the environment,$_SERVER['APP_DEBUG']can trigger a notice. Using a default keeps the bootstrap robust:-if ($_SERVER['APP_DEBUG']) { +if (($_SERVER['APP_DEBUG'] ?? false)) { umask(0000); }symfony/templates/index.html.twig (1)
32-47: Consider usingPromise.allorfor...ofinstead offorEachwith async.While the current implementation works,
forEachdoesn't await async callbacks. If you need sequential execution or want to handle errors collectively, consider:- targets.forEach(async ({ id, url }) => { + for (const { id, url } of targets) { const container = document.getElementById(id); if (!container) { - return; + continue; } try { const response = await fetch(url, { headers: { 'X-Debug': 'ajax-check' } }); const contentType = response.headers.get('content-type') || 'unknown'; const bodyText = await response.text(); container.textContent = `Content-Type: ${contentType} | Body: ${bodyText}`; } catch (error) { container.textContent = `Failed to load: ${error}`; } - }); + }Alternatively, if parallel execution is desired (current behavior), make the intent explicit with
Promise.all:await Promise.all(targets.map(async ({ id, url }) => { // ... existing logic }));symfony/templates/base.html.twig (1)
8-139: Consider externalizing CSS for production.While inline styles work well for this demo, consider moving the CSS to an external stylesheet (e.g.,
assets/styles/app.css) for better maintainability, caching, and separation of concerns in a production environment.This is a minor maintainability improvement and can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
symfony/composer.lockis excluded by!**/*.locksymfony/symfony.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
.gitignore(1 hunks).rr.yaml(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)main.go(2 hunks)main_test.go(1 hunks)php-conf/10-app.ini(1 hunks)plugin.go(5 hunks)plugin_test.go(1 hunks)request_parser.go(1 hunks)request_parser_test.go(1 hunks)symfony/.editorconfig(1 hunks)symfony/.env(1 hunks)symfony/.gitignore(1 hunks)symfony/assets/app.js(1 hunks)symfony/assets/controllers.json(1 hunks)symfony/assets/controllers/csrf_protection_controller.js(1 hunks)symfony/assets/controllers/hello_controller.js(1 hunks)symfony/assets/stimulus_bootstrap.js(1 hunks)symfony/assets/styles/app.css(1 hunks)symfony/bin/console(1 hunks)symfony/bin/phpunit(1 hunks)symfony/composer.json(1 hunks)symfony/config/bundles.php(1 hunks)symfony/config/packages/asset_mapper.yaml(1 hunks)symfony/config/packages/cache.yaml(1 hunks)symfony/config/packages/csrf.yaml(1 hunks)symfony/config/packages/debug.yaml(1 hunks)symfony/config/packages/doctrine.yaml(1 hunks)symfony/config/packages/doctrine_migrations.yaml(1 hunks)symfony/config/packages/framework.yaml(1 hunks)symfony/config/packages/mailer.yaml(1 hunks)symfony/config/packages/messenger.yaml(1 hunks)symfony/config/packages/monolog.yaml(1 hunks)symfony/config/packages/notifier.yaml(1 hunks)symfony/config/packages/nyholm_psr7.yaml(1 hunks)symfony/config/packages/property_info.yaml(1 hunks)symfony/config/packages/routing.yaml(1 hunks)symfony/config/packages/security.yaml(1 hunks)symfony/config/packages/translation.yaml(1 hunks)symfony/config/packages/twig.yaml(1 hunks)symfony/config/packages/ux_turbo.yaml(1 hunks)symfony/config/packages/validator.yaml(1 hunks)symfony/config/packages/web_profiler.yaml(1 hunks)symfony/config/preload.php(1 hunks)symfony/config/reference.php(1 hunks)symfony/config/routes.yaml(1 hunks)symfony/config/routes/framework.yaml(1 hunks)symfony/config/routes/security.yaml(1 hunks)symfony/config/routes/web_profiler.yaml(1 hunks)symfony/config/services.yaml(1 hunks)symfony/importmap.php(1 hunks)symfony/phpunit.dist.xml(1 hunks)symfony/public/index.php(1 hunks)symfony/src/Controller/IndexController.php(1 hunks)symfony/src/Kernel.php(1 hunks)symfony/templates/base.html.twig(1 hunks)symfony/templates/form_multipart.html.twig(1 hunks)symfony/templates/form_multipart_result.html.twig(1 hunks)symfony/templates/form_urlencoded.html.twig(1 hunks)symfony/templates/form_urlencoded_result.html.twig(1 hunks)symfony/templates/index.html.twig(1 hunks)symfony/tests/bootstrap.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
symfony/public/index.php (1)
symfony/src/Kernel.php (1)
Kernel(8-31)
plugin.go (1)
request_parser.go (1)
Uploads(36-39)
plugin_test.go (2)
plugin.go (2)
Logger(42-44)Plugin(31-39)request_parser.go (1)
Uploads(36-39)
🪛 dotenv-linter (4.0.0)
symfony/.env
[warning] 33-33: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🪛 golangci-lint (2.5.0)
request_parser.go
[error] 121-121: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
[error] 143-143: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
plugin_test.go
[warning] 24-24: unused-parameter: parameter 'name' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 50-50: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 80-80: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 82-82: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[error] 363-363: S1009: should omit nil check; len() for nil maps is defined as zero
(staticcheck)
🪛 LanguageTool
README.md
[grammar] ~8-~8: Ensure spelling is correct
Context: ...Symfony app to validate HTML responses, urlencoded forms, multipart uploads, and assorted ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (62)
symfony/config/routes/security.yaml (1)
1-3: Verify the referenced service is properly configured.The route configuration references the service
security.route_loader.logoutwhich should be defined in your Symfony service container. Ensure this service is properly configured elsewhere in your application's service definitions (typically inservices.yamlor similar configuration files).symfony/config/packages/translation.yaml (1)
1-5: Symfony translation configuration is correct and follows best practices.The configuration properly sets up the translation framework with a sensible default locale and path structure using Symfony's standard parameter syntax. The empty
providersmapping is acceptable since translation providers are optional.symfony/config/packages/doctrine_migrations.yaml (1)
1-6: Configuration looks good for Lambda deployment.This is a minimal and correct Doctrine Migrations setup. Notably,
enable_profiler: falseis an appropriate optimization for Lambda environments where memory and execution time are constrained. The namespace convention is correctly documented.symfony/config/packages/validator.yaml (1)
1-11: This appears to be a Symfony sample app config disconnected from the stated PR objective.The validator configuration is syntactically valid, but this file seems orthogonal to the PR's stated goal of "content type handling for forms, uploads, and base64-encoded content." This appears to be part of a Symfony sample application included alongside the core Lambda feature implementation.
The commented auto_mapping section and test-environment override are reasonable defaults. However, if the sample app uses Doctrine entities with validation constraints, consider uncommenting and configuring auto_mapping to leverage automatic constraint inference.
symfony/config/packages/asset_mapper.yaml (1)
1-11: Well-configured asset mapper setup with environment-specific behavior.The configuration is syntactically correct and follows Symfony best practices: strict mode in development catches missing imports early, while lenient
warnmode in production prevents site breakage. Theassets/directory path is a standard convention.Verify that the
assets/directory is present in the project structure and properly included in the Lambda deployment package, as this configuration depends on that directory being accessible at runtime.symfony/config/reference.php (2)
1534-1545: Inconsistency between AI summary and actual code.The AI-generated summary mentions a method named
App::coderabbit_config, but the actual code implementsApp::config. This discrepancy suggests the summary may be inaccurate or based on outdated information.
1618-1621: Pass-through method provides only type checking.The
Routes::config()method simply returns the input array without any transformation or validation. While this is likely intentional to provide type safety through Psalm annotations, it adds no runtime logic or validation.symfony/config/packages/security.yaml (2)
1-28: Security configuration looks solid for a sample application.The firewall setup follows Symfony best practices:
- Dev firewall correctly excludes profiler, debugging tools, and static assets with
security: false- Main firewall uses lazy loading and an in-memory provider, appropriate for this sample app
- Password hashing uses the 'auto' strategy for compatibility
For production deployments, consider moving away from in-memory user providers and adding explicit access control rules beyond the commented examples.
30-39: The password hasher configuration values are appropriate for testing and align with Symfony's documented examples. The configuredtime_cost: 3andmemory_cost: 10appear in Symfony's own security documentation as acceptable low-cost values for test environments. The actual minimum values for argon2 depend on the underlying PHP provider and can vary; some implementations accepttime_cost ≥ 1andmemory_cost ≥ 8 KiB, while others require higher minimums. The comments claiming "lowest possible value" are overstated without platform/provider context, but the configuration itself does not require changes.Likely an incorrect or invalid review comment.
.gitignore (1)
18-18: LGTM!Adding
vendor/and.cacheto .gitignore is appropriate for keeping dependency artifacts and build outputs out of version control.Also applies to: 23-23
symfony/config/packages/property_info.yaml (1)
1-3: LGTM!Enabling the constructor extractor in Symfony's property info component is a standard configuration that allows property metadata extraction from constructor parameters.
symfony/config/routes.yaml (1)
1-11: LGTM!The routing configuration is well-documented and follows Symfony conventions. The schema reference on Line 1 provides helpful IDE support.
symfony/public/index.php (1)
1-9: LGTM!The front controller follows the Symfony Runtime component pattern correctly. The factory function creates a Kernel instance with environment and debug settings from the context, which integrates well with the Lambda-compatible Kernel implementation that uses
SYMFONY_CACHE_DIRandSYMFONY_LOG_DIRenvironment variables.symfony/assets/controllers.json (1)
1-15: LGTM!The Symfony UX Turbo controller configuration is properly structured. Enabling turbo-core with eager fetch and disabling mercure-turbo-stream are reasonable defaults.
symfony/config/packages/ux_turbo.yaml (1)
1-4: LGTM!Enabling header-based CSRF protection is appropriate for stateless environments like AWS Lambda. This aligns well with the serverless deployment model where session-based CSRF tokens would be problematic.
symfony/config/packages/twig.yaml (1)
1-6: LGTM!The Twig configuration is well-structured. Enabling strict variables in the test environment (Lines 4-6) is a best practice that helps catch undefined variable bugs during testing.
.rr.yaml (2)
6-9: LGTM!The environment variables align well with the Symfony Kernel implementation. Setting
SYMFONY_CACHE_DIRandSYMFONY_LOG_DIRto/tmpdirectories is appropriate for Lambda's read-only filesystem, and theAPP_RUNTIMEconfigures the RoadRunner-Symfony integration correctly.
12-13: Verify log configuration for production deployment.The logging mode has been changed from
production/errortodevelopment/debug. Debug-level logging can expose sensitive information and impact performance in production environments.Confirm whether:
- This configuration is intended only for development/testing
- There's a separate production configuration that overrides these settings
- The Lambda deployment uses environment-specific RoadRunner configs
If this file is used in production, consider reverting to production log levels or using environment variables to control log verbosity.
symfony/config/packages/csrf.yaml (1)
1-11: LGTM! Stateless CSRF configuration is appropriate for Lambda environments.The stateless CSRF protection configuration aligns well with the serverless/Lambda deployment model where traditional session-based CSRF tokens wouldn't persist across invocations.
main.go (1)
23-23: LGTM! Environment setup refactored into a dedicated function.Extracting environment configuration into
configureEnvironment()improves code organization and testability.main_test.go (1)
9-24: LGTM! Test coverage for environment configuration is thorough.The test properly uses
t.Setenvfor automatic cleanup and validates both PATH appending and LD_LIBRARY_PATH setting.symfony/config/packages/cache.yaml (1)
1-19: LGTM! Configuration scaffolding provides helpful examples.This file provides commented configuration examples for various cache backends without enabling any by default, which is appropriate for a template/example setup.
symfony/assets/stimulus_bootstrap.js (1)
1-5: LGTM! Standard Stimulus bootstrap implementation.The Stimulus initialization follows Symfony UX conventions and includes a helpful placeholder comment for registering custom controllers.
symfony/config/routes/framework.yaml (1)
1-4: LGTM! Dev-only error routing is properly scoped.The error route configuration is correctly limited to the dev environment with the
when@devdirective, preventing it from being active in production.symfony/.editorconfig (1)
1-17: LGTM! EditorConfig settings follow standard conventions.The configuration provides sensible defaults with appropriate overrides for YAML compose files (2-space indent) and Markdown files (preserve trailing whitespace).
plugin.go (3)
137-154: LGTM: Base64 decoding and cleanup handling.The base64 decoding logic correctly handles encoded request bodies, and the deferred cleanup pattern ensures uploaded files are properly removed even if errors occur. Good defensive programming.
328-363: LGTM: Content type handling with proper cleanup.The
transformBodymethod correctly handles different content types (none, URL-encoded, multipart) and returns appropriate cleanup functions for file uploads. The error handling is comprehensive.
365-373: LGTM: URI construction helper.The
buildURIhelper correctly constructs URIs with optional query strings and handles the empty path case by defaulting to "/".php-conf/10-app.ini (1)
19-19: Verify preload file exists.The OPcache preload references
/var/task/config/preload.php. Ensure this file exists in the Symfony application.Dockerfile (2)
107-111: Verify the lambda directory structure.The
COPY --exclude="*.new" ./lambda/* .command copies files from alambda/directory that isn't visible in this PR. Please ensure this directory exists and contains the necessarymain.goandplugin.gofiles.
4-4: Go version 1.25.5 is valid and current. This is the latest stable Go release as of December 2025 (released December 2, 2025), so no change is needed.symfony/composer.json (1)
1-108: Well-structured Symfony project configuration.The composer.json follows Symfony best practices with appropriate dependency declarations, autoload configuration, and polyfill replacements for PHP 8.2+. The RoadRunner integration via
runtime/roadrunner-symfony-nyholmaligns with the PR's Lambda support objective.symfony/assets/controllers/csrf_protection_controller.js (1)
1-81: Solid CSRF protection implementation for Turbo integration.The double-submit cookie pattern with SameSite=strict and conditional Secure flag is correctly implemented. Token validation via regex and proper cookie cleanup on submission complete the security model.
plugin_test.go (2)
102-369: Comprehensive test coverage for Lambda request handling.The table-driven tests cover a wide range of content types including JSON, URL-encoded forms, multipart uploads, base64-encoded bodies, and binary data. The assertions validate method, URI, query string, headers, body transformation, and upload metadata propagation.
253-255: Loop variable capture is no longer needed in Go 1.22+.The
tt := ttpattern was necessary before Go 1.22 to capture the loop variable for parallel subtests. If the project uses Go 1.22+, this line can be removed.symfony/config/packages/nyholm_psr7.yaml (1)
1-11: Correct PSR-17 factory registration.The configuration properly maps all six PSR-17 HTTP factory interfaces to the Nyholm implementation, enabling autowiring throughout the Symfony application.
request_parser.go (1)
1-408: Well-structured request parsing implementation.The parser correctly handles URL-encoded and multipart form data with proper tree-building logic for nested PHP-style field names. The
maxLevelconstant protects against deep nesting attacks, and temporary file cleanup viaUploads.Clear()prevents resource leaks.request_parser_test.go (2)
32-60: URL-encoded parsing test covers nested keys and arrays wellThis test exercises plain fields, nested bracket syntax, and repeated keys into arrays; good coverage and assertions look correct.
131-139: Empty dataTree packing behavior is well specifiedAsserting that an empty
dataTree{}packs to{}makes the intended wire format explicit; no issues here.symfony/assets/styles/app.css (1)
1-3: Simple demo styling is fineMinimal body background color is appropriate for a sample app; nothing to change here.
symfony/config/packages/mailer.yaml (1)
1-3: Mailer DSN wiring looks standardUsing
framework.mailer.dsn: '%env(MAILER_DSN)%'is the usual Symfony pattern and keeps transport fully env-driven; looks good.symfony/config/routes/web_profiler.yaml (1)
1-8: Dev-only web profiler routing matches standard setup
when@devwith/_wdtand/_profilerroutes mirrors the usual Symfony skeleton; no issues.symfony/config/packages/debug.yaml (1)
1-5: VarDumper server config is conventional and well-documentedDev-only
dump_destinationpointing totcp://%env(VAR_DUMPER_SERVER)%with comments matches common Symfony debug setups; no changes needed.symfony/config/packages/routing.yaml (1)
1-10: Routing defaults and prod override look correctUsing
default_urifromDEFAULT_URIand relaxingstrict_requirementsonly in prod is consistent with Symfony’s recommended patterns.symfony/bin/phpunit (1)
1-4: Add a guard for missing PHPUnit binary for clearer failuresRight now, if
vendor/phpunit/phpunit/phpunitis missing (e.g., beforecomposer install), this will emit a fatal error without context. A small guard makes failures clearer:-#!/usr/bin/env php -<?php - -require dirname(__DIR__).'/vendor/phpunit/phpunit/phpunit'; +#!/usr/bin/env php +<?php + +$phpunit = dirname(__DIR__).'/vendor/phpunit/phpunit/phpunit'; + +if (!is_file($phpunit)) { + fwrite(STDERR, "PHPUnit binary not found at {$phpunit}. Did you run 'composer install'?\n"); + exit(1); +} + +require $phpunit;symfony/config/packages/framework.yaml (1)
1-15: LGTM!The framework configuration follows Symfony best practices with appropriate environment variable usage for secrets and lazy session initialization. The test-specific mock session storage is correctly configured for the test environment.
symfony/config/packages/web_profiler.yaml (1)
1-13: LGTM!The environment-specific web profiler configuration is appropriate, enabling the toolbar for development while disabling profiler collection in tests for better performance.
symfony/config/bundles.php (1)
3-16: LGTM!The bundle configuration follows Symfony best practices with appropriate environment restrictions. Development and debugging tools are correctly limited to dev/test environments, while core application bundles are enabled for all environments.
symfony/templates/form_urlencoded_result.html.twig (1)
1-23: LGTM!The template is well-structured with proper Twig inheritance, automatic escaping for XSS protection, and appropriate handling of both scalar and iterable values. The user experience is enhanced with an empty state message and navigation back to the form.
symfony/phpunit.dist.xml (1)
1-44: LGTM!The PHPUnit configuration is comprehensive and follows best practices with strict error handling, proper test environment setup, and appropriate deprecation trigger mappings for Doctrine. The configuration will help catch issues early during development.
symfony/config/packages/doctrine.yaml (3)
5-7: Ensure database server version is configured.The commented
server_versionmust be configured either here or in theDATABASE_URLenvironment variable. Without this, Doctrine may make incorrect assumptions about database features and compatibility.Verify that your
DATABASE_URLincludes the server version (e.g.,postgresql://user:pass@host/db?serverVersion=16) or uncomment and set theserver_versionhere:# IMPORTANT: You MUST configure your server version, # either here or in the DATABASE_URL env var (see .env file) - #server_version: '16' + server_version: '16'
13-14: Verify PostgreSQL as the intended database platform.The configuration explicitly sets identity generation preferences for PostgreSQL. If PostgreSQL is your intended production database, this is fine. However, if you plan to support multiple database platforms, you may want to either remove this platform-specific configuration or add preferences for other platforms.
If PostgreSQL is the only intended database, consider adding a comment to document this decision:
naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware + # This application requires PostgreSQL identity_generation_preferences: Doctrine\DBAL\Platforms\PostgreSQLPlatform: identity
32-48: LGTM!The production cache configuration follows Doctrine best practices with separate cache pools for query and result caching. This will provide appropriate performance optimization in production while keeping development environments uncached for easier debugging.
symfony/assets/controllers/hello_controller.js (1)
1-16: LGTM! Example Stimulus controller.This is a standard Stimulus controller example. The implementation is correct and serves well as a starting point for custom controllers.
symfony/templates/form_multipart_result.html.twig (1)
1-35: LGTM! Well-structured result template.The template properly handles both iterable and scalar field values and displays file metadata appropriately. Twig's auto-escaping protects against XSS.
README.md (1)
1-109: Excellent documentation!The README provides comprehensive guidance on building, testing, and deploying the Lambda integration. The request flow diagram and step-by-step instructions are particularly helpful.
Note: The static analysis hint about "urlencoded" is a false positive—this is standard technical terminology for URL-encoded content.
symfony/templates/base.html.twig (1)
1-162: Well-structured base template with inline styles.The template provides a solid foundation with proper block structure and responsive navigation. The inline CSS is acceptable for a demo application.
symfony/config/packages/messenger.yaml (2)
15-15: Verify Doctrine configuration for failed transport.The
failedtransport usesdoctrine://default, which requires Doctrine DBAL to be installed and configured with a valid database connection.
7-14: Verify PostgreSQL usage and consider transport portability.The
use_notify: trueoption requires PostgreSQL's LISTEN/NOTIFY feature and will fail with MySQL, MariaDB, or SQLite. Ensure your deployment uses PostgreSQL, or remove this option for database portability.Run the following script to verify the database configuration and messenger transport setup:
#!/bin/bash # Check DATABASE_URL and MESSENGER_TRANSPORT_DSN configuration # Look for database configuration in .env files echo "=== Checking database configuration ===" fd -t f '^\.env' -X cat {} \; | grep -E '(DATABASE_URL|MESSENGER_TRANSPORT_DSN)' # Check if Doctrine is configured echo -e "\n=== Checking Doctrine configuration ===" cat symfony/config/packages/doctrine.yaml 2>/dev/null || echo "doctrine.yaml not found" # Search for doctrine/dbal in composer files echo -e "\n=== Checking Doctrine DBAL dependency ===" fd 'composer.json' symfony -X cat {} \; | jq -r '.require["doctrine/doctrine-bundle"], .require["doctrine/dbal"]' 2>/dev/null || echo "No Doctrine dependencies found in composer.json"symfony/src/Controller/IndexController.php (3)
13-17: LGTM!The index route correctly renders the template with proper attribute routing.
19-23: LGTM!The ping endpoint correctly returns a plain text response with appropriate headers.
25-29: LGTM!The health check endpoint correctly returns a JSON response with appropriate status.
| decoded, _ := url.QueryUnescape(v.Value) | ||
| resp[v.Name] = &httpV1proto.HeaderValue{ | ||
| Value: [][]byte{[]byte(v.Value)}, | ||
| Value: [][]byte{[]byte(decoded)}, |
There was a problem hiding this comment.
Handle QueryUnescape errors properly.
The error from url.QueryUnescape(v.Value) is silently ignored. If unescaping fails, you should fall back to the original value or log the error.
Apply this diff to handle the error:
- decoded, _ := url.QueryUnescape(v.Value)
+ decoded, err := url.QueryUnescape(v.Value)
+ if err != nil {
+ log.Warn("failed to unescape cookie value", zap.String("name", v.Name), zap.Error(err))
+ decoded = v.Value
+ }
resp[v.Name] = &httpV1proto.HeaderValue{
Value: [][]byte{[]byte(decoded)},
}📝 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.
| decoded, _ := url.QueryUnescape(v.Value) | |
| resp[v.Name] = &httpV1proto.HeaderValue{ | |
| Value: [][]byte{[]byte(v.Value)}, | |
| Value: [][]byte{[]byte(decoded)}, | |
| decoded, err := url.QueryUnescape(v.Value) | |
| if err != nil { | |
| log.Warn("failed to unescape cookie value", zap.String("name", v.Name), zap.Error(err)) | |
| decoded = v.Value | |
| } | |
| resp[v.Name] = &httpV1proto.HeaderValue{ | |
| Value: [][]byte{[]byte(decoded)}, |
🤖 Prompt for AI Agents
In plugin.go around lines 268 to 270, the call to url.QueryUnescape ignores the
returned error; change it to capture the error (decoded, err :=
url.QueryUnescape(v.Value)) and handle it by falling back to the original
v.Value when err != nil (i.e., use v.Value as the value instead of decoded), and
optionally log the error via the package's logger if available; construct
resp[v.Name] with the chosen value bytes (either decoded on success or v.Value
on error).
* fixes lambda support for forms, uploads and other base64-encoded content
5b51886 to
bdee797
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
request_parser.go (1)
320-323: Potential panic from unchecked type assertion remains unaddressed.This unchecked type assertion can panic if the tree structure is inconsistent. Use a safe type assertion as previously suggested.
- return ft[i[0]].(fileTree).mount(i[1:], v) + sub, ok := ft[i[0]].(fileTree) + if !ok { + return errors.New("invalid tree structure: expected fileTree") + } + return sub.mount(i[1:], v)
🧹 Nitpick comments (6)
main.go (1)
86-93: Hardcoded Symfony directories reduce reusability.This AWS Lambda plugin appears to be a generic RoadRunner integration, but
configureEnvironment()creates Symfony-specific directories. Consider making these paths configurable via environment variables or removing them from this generic plugin, delegating directory creation to the PHP application bootstrap.plugin_test.go (2)
85-100: Unsafe reflection to set unexported fields is fragile.Using
unsafe.Pointerto set unexported fields inpoolImp.PExeccreates a maintenance burden—any field rename or reorder in the upstream library will silently break these tests. Consider checking if the library provides a constructor or test helper, or document this fragility with a comment explaining the workaround.
362-366: Simplify nil check beforelen().The nil check is redundant since
len()on a nil slice returns 0.if len(tt.cookies) > 0 { - if gotReq.Cookies == nil || len(gotReq.Cookies) == 0 { + if len(gotReq.Cookies) == 0 { t.Fatalf("expected cookies to be parsed") } }request_parser.go (3)
120-124: UseNewRequestWithContextfor proper context propagation.Static analysis correctly flags that
http.NewRequestshould be replaced withhttp.NewRequestWithContextto support cancellation and timeouts. Since this is called from a Lambda handler context, propagating the context is important.-func parseURLEncoded(body []byte, headers map[string]string) ([]byte, error) { - req, err := http.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) +func parseURLEncoded(ctx context.Context, body []byte, headers map[string]string) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/", bytes.NewReader(body))
142-146: Same context propagation issue here.Apply the same fix as
parseURLEncodedto accept a context parameter and usehttp.NewRequestWithContext.-func parseMultipart(body []byte, headers map[string]string) ([]byte, *Uploads, error) { - req, err := http.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) +func parseMultipart(ctx context.Context, body []byte, headers map[string]string) ([]byte, *Uploads, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/", bytes.NewReader(body))
160-162: Consider logging upload open failures.While
f.Errorcaptures the error code for the PHP side, silently discarding the Go error makes debugging harder. Consider logging the error at debug level.for _, f := range uploads.list { - _ = f.Open() + if err := f.Open(); err != nil { + // Error code already set in f.Error for PHP side + // Log for debugging purposes (if logger available) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks).rr.yaml(1 hunks)Dockerfile(1 hunks)README.md(1 hunks)main.go(2 hunks)main_test.go(1 hunks)plugin.go(6 hunks)plugin_test.go(1 hunks)request_parser.go(1 hunks)request_parser_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Dockerfile
- request_parser_test.go
- .rr.yaml
- main_test.go
- plugin.go
🧰 Additional context used
🧬 Code graph analysis (1)
plugin_test.go (2)
plugin.go (2)
Logger(42-44)Plugin(31-39)request_parser.go (1)
Uploads(36-39)
🪛 golangci-lint (2.5.0)
plugin_test.go
[warning] 24-24: unused-parameter: parameter 'name' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 50-50: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 80-80: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 82-82: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
[error] 363-363: S1009: should omit nil check; len() for nil maps is defined as zero
(staticcheck)
request_parser.go
[error] 121-121: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
[error] 143-143: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
🪛 LanguageTool
README.md
[grammar] ~8-~8: Ensure spelling is correct
Context: ...Symfony app to validate HTML responses, urlencoded forms, multipart uploads, and assorted ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (9)
.gitignore (2)
18-18: Uncomment vendor/ to ignore Go dependencies directory.This change aligns with typical Go project patterns where the
vendor/directory is not committed to source control. Projects rely ongo.modandgo.sumfor reproducibility instead.
23-23: Add .cache to ignore build and test artifacts.The addition of
.cachealigns well with the PR's introduction of Docker containerization and test infrastructure, which commonly generate temporary cache files.main.go (1)
82-85: LGTM!The environment variable configuration follows standard AWS Lambda bootstrap patterns. Ignoring
os.Setenverrors is acceptable here as these calls rarely fail on Linux.README.md (1)
1-59: LGTM!Documentation is clear, well-structured, and provides a good overview of the project with helpful diagrams and build instructions. The term "urlencoded" on line 8 is standard technical terminology for
application/x-www-form-urlencoded.plugin_test.go (2)
102-251: Good test coverage with comprehensive scenarios.The table-driven test structure covers a wide range of HTTP scenarios including base64 decoding, multipart uploads, URL-encoded forms, and various content types. The test cases properly verify method, URI, query string, headers, body transformation, and upload handling.
371-407: LGTM!The proto response handling test properly validates status code, headers, and body extraction from the protobuf response structure.
request_parser.go (3)
46-52: LGTM!The cleanup method properly iterates over uploads and removes temporary files. The
exists()check before removal is a minor TOCTOU race but benign sinceos.Removeon a non-existent file is safe.
73-96: LGTM!The
Openmethod properly handles the file upload workflow with appropriate error codes, deferred cleanup of file handles, and size tracking fromio.Copy.
356-395: LGTM!The generic
prepareTreeNodefunction properly handles edge cases in tree building with clear error messages. The complexity reflects the PHP form parsing semantics being mirrored.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugin_test.go (2)
102-369: Strong request-translation coverage; consider a few extra assertionsThe table-driven
TestHandlerBuildsLambdaRequestsexercises a wide matrix of methods, content-types, base64 handling, and uploads, and validates URI, raw query, content-type, decoded/normalized bodies, uploads metadata, and cookie presence. To tighten coverage further without adding much noise, you might:
- For some non‑
hasExpectedBodycases (e.g.,postJSON,graphql,htmlBody,pngBody), also assert on the forwarded body where semantics are simple (e.g., unchanged passthrough), to catch regressions intransformBody.- In the
graphqlcase, assert thatgotReq.Cookiescontains the expected key/value pairs, not just that the slice is non‑empty, to validate cookie parsing more thoroughly.These are optional, but they’d make the tests even more resilient to subtle regressions in request normalization.
371-407: Also assert propagation of non-Content-Type headers in proto response test
TestHandleProtoResponsevalidates status,Content-Type, and body mapping fromhttpV1proto.Responsetoevents.APIGatewayV2HTTPResponse. Since you already include an extra"X-Test"header inrspProto, it would be valuable to assert that it appears inresponse.Headersto cover generic header propagation, not just content-type. For example:if response.Body != `{"ok":true}` { t.Fatalf("body mismatch: got %s", response.Body) } + + if response.Headers["X-Test"] != "value" { + t.Fatalf("X-Test header mismatch: got %s", response.Headers["X-Test"]) + }This keeps the test focused while ensuring
handlePROTOresponsedoesn’t accidentally drop arbitrary headers.request_parser.go (1)
73-96: IgnoringFileUpload.Openerrors may hide upload failures
FileUpload.Opencorrectly setsErrortouploadErrorNoFile,uploadErrorNoTmpDir, oruploadErrorCantWriteon failure, butparseMultipartcurrently invokes it as:for _, f := range uploads.list { _ = f.Open() }This means any failure to materialize an upload to disk is silently ignored at the parser level; downstream code must remember to consult the
Errorfield on eachFileUploadto detect problems.If the intent is to fail the whole request when any upload cannot be persisted, you might instead aggregate and propagate those errors (or at least log them). If per-file error reporting is desired and the overall parse should still succeed, the current approach is fine but could benefit from a brief comment to document that design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
main.go(2 hunks)plugin_test.go(1 hunks)request_parser.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- main.go
🧰 Additional context used
🧬 Code graph analysis (1)
plugin_test.go (2)
plugin.go (2)
Logger(42-44)Plugin(31-39)request_parser.go (1)
Uploads(36-39)
🪛 golangci-lint (2.5.0)
request_parser.go
[error] 120-120: undefined: context
(typecheck)
[error] 142-142: undefined: context
(typecheck)
plugin_test.go
[error] 120-120: : # github.com/roadrunner-server/aws-lambda [github.com/roadrunner-server/aws-lambda.test]
./request_parser.go:120:26: undefined: context
./request_parser.go:142:25: undefined: context
./plugin.go:412:35: not enough arguments in call to parseURLEncoded
have ([]byte, map[string]string)
want (unknown type, []byte, map[string]string)
./plugin.go:418:43: not enough arguments in call to parseMultipart
have ([]byte, map[string]string)
want (unknown type, []byte, map[string]string)
./request_parser_test.go:35:40: not enough arguments in call to parseURLEncoded
have ([]byte, map[string]string)
want (unknown type, []byte, map[string]string)
./request_parser_test.go:90:55: not enough arguments in call to parseMultipart
have ([]byte, map[string]string)
want (unknown type, []byte, map[string]string)
(typecheck)
🔇 Additional comments (3)
plugin_test.go (1)
22-100: Test helper scaffolding is robust and appropriate for isolationThe
namedLoggerStub,fakePool.Exec, andbuildPExec/setUnexportedFieldhelpers cleanly decouple tests from real pool behavior while still exercising the protobuf payload and uploads paths. The use ofappend([]byte(nil), ...)to copy bodies/uploads and the mutex infakePoolavoid accidental aliasing and races. The reliance on unexportedPExecfields is brittle but acceptable in tests, since breakage will surface immediately if upstream internals change.request_parser.go (2)
240-327: Tree-building logic mirrors roadrunner-http and now handles invalid structures more safelyThe
dataTree.push/mountandfileTree.push/mountfunctions, together withfetchIndexesandprepareTreeNode, provide a faithful port of the nested form-key handling from roadrunner HTTP. The updatedfileTree.mountimplementation that uses a safe type assertion:sub, ok := ft[i[0]].(fileTree) if !ok { return errors.New("invalid tree structure: expected fileTree") }avoids the panic risk highlighted in the earlier review while still surfacing structural inconsistencies as errors. The combination of
maxLevel,prepareTreeNodeinvariants, andisDataEmptycovers a good range of edge cases for conflicting or malformed keys. No changes needed here.
3-12: The referenced functionsparseURLEncodedandparseMultipartdo not exist in the current request_parser.go codebase.This review comment references functions and modifications that were reverted in commit a0526a1 ("fix: revert to original arguments vs. requestwithcontext"). The file's current state does not contain
parseURLEncodedorparseMultipartfunctions withcontext.Contextparameters, and thecontextimport is correctly absent since it is not used.If the intent is to re-apply the context.Context parameter changes that were previously reverted, this review comment should be updated to clarify that it represents a reversal of the revert, or the code changes need to be provided for evaluation.
| func parseMultipart(ctx context.Context, body []byte, headers map[string]string) ([]byte, *Uploads, error) { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/", bytes.NewReader(body)) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| for k, v := range headers { | ||
| req.Header.Add(k, v) | ||
| } | ||
| req.ContentLength = int64(len(body)) | ||
|
|
||
| if err := req.ParseMultipartForm(defaultMaxMemory); err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| uploads, err := parseUploads(req) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| for _, f := range uploads.list { | ||
| _ = f.Open() | ||
| } | ||
|
|
||
| data, err := parseMultipartData(req) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| encoded, err := packDataTree(data) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| return encoded, uploads, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "request_parser.go"Repository: roadrunner-server/aws-lambda
Length of output: 89
🏁 Script executed:
cat -n request_parser.go | head -200Repository: roadrunner-server/aws-lambda
Length of output: 5256
🏁 Script executed:
wc -l request_parser.goRepository: roadrunner-server/aws-lambda
Length of output: 93
🏁 Script executed:
rg "func parseMultipart" request_parser.go -A 5Repository: roadrunner-server/aws-lambda
Length of output: 513
🏁 Script executed:
git log --oneline request_parser.go | head -20Repository: roadrunner-server/aws-lambda
Length of output: 136
🏁 Script executed:
git show a0526a1 --statRepository: roadrunner-server/aws-lambda
Length of output: 957
🏁 Script executed:
rg "MultipartForm|RemoveAll|defer|Close" request_parser.goRepository: roadrunner-server/aws-lambda
Length of output: 320
🌐 Web query:
Go http.Request ParseMultipartForm RemoveAll temp files cleanup
💡 Result:
Yes — ParseMultipartForm may write large file parts to disk, and you (the handler) should remove those temp files when done.
Recommended pattern:
- After parsing, defer removal (check for nil):
- if err := r.ParseMultipartForm(maxMemory); err == nil {
defer func() {
if r.MultipartForm != nil {
_ = r.MultipartForm.RemoveAll()
}
}()
}
- if err := r.ParseMultipartForm(maxMemory); err == nil {
- Or call r.MultipartForm.RemoveAll() explicitly once you’ve finished using uploaded data.
Notes:
- Memory buffers become GC‑eligible; temp files are removed by RemoveAll. The server may attempt cleanup, but it’s the caller’s safe practice to call RemoveAll (defer) to guarantee cleanup. [1][2]
Sources:
[1] net/http ParseMultipartForm docs & tests (shows use of MultipartForm.RemoveAll).
[2] Discussion / Q&A on when temporary files are removed (Stack Overflow / Go issues).
🏁 Script executed:
cat -n request_parser.go | sed -n '142,175p'Repository: roadrunner-server/aws-lambda
Length of output: 1026
Call MultipartForm.RemoveAll to avoid leaking net/http temp files
parseMultipart calls ParseMultipartForm at line 152 and then builds your own Uploads structure and data tree. However, it never calls req.MultipartForm.RemoveAll(), so the temporary files created by ParseMultipartForm are left on disk for the lifetime of the process. Since FileUpload.Open creates additional temp files that you manage via Uploads.Clear, this results in unnecessary and potentially unbounded temp-file accumulation.
Add a defer statement after the successful ParseMultipartForm call to clean up the net/http-managed files:
func parseMultipart(body []byte, headers map[string]string) ([]byte, *Uploads, error) {
req, err := http.NewRequest(http.MethodPost, "/", bytes.NewReader(body))
if err != nil {
return nil, nil, err
}
for k, v := range headers {
req.Header.Add(k, v)
}
req.ContentLength = int64(len(body))
if err := req.ParseMultipartForm(defaultMaxMemory); err != nil {
return nil, nil, err
}
+ defer req.MultipartForm.RemoveAll()This keeps only your own temp copies (referenced by TempFilename) around, which Uploads.Clear can later delete, and prevents /tmp from filling up under sustained traffic.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 142-142: undefined: context
(typecheck)
🤖 Prompt for AI Agents
In request_parser.go around lines 142 to 175, after the successful call to
req.ParseMultipartForm(defaultMaxMemory) add a deferred cleanup of the
net/http-managed temp files so they don’t leak; specifically, immediately after
ParseMultipartForm returns nil, defer a safe call that checks req.MultipartForm
for nil and calls req.MultipartForm.RemoveAll() (e.g. defer func(){ if
req.MultipartForm != nil { _ = req.MultipartForm.RemoveAll() } }()), leaving
your existing Uploads.Clear logic to manage your own temp copies.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.