Conversation
📝 WalkthroughWalkthroughAdds tenant quota enforcement: new Changes
Sequence DiagramsequenceDiagram
participant Entity as Entity (FAQ/Category/User/Attachment)
participant Enforcer as TenantQuotaEnforcer
participant Resolver as TenantContextResolver
participant DB as DatabaseDriver
Entity->>Enforcer: getTenantQuotaEnforcer()
Enforcer->>Resolver: resolveTenantContext()
Resolver-->>Enforcer: TenantContext
Enforcer->>DB: run COUNT/SUM queries for tenant
DB-->>Enforcer: current_count_or_size
Entity->>Enforcer: assertCanCreate... / assertCanStoreAttachment(size)
alt quota exceeded
Enforcer-->>Entity: throw QuotaExceededException
else within quota
Enforcer-->>Entity: return (void)
Entity->>DB: proceed with insert/store
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 30: Remove the incomplete changelog list item: locate the lone
bullet-only entry (a line containing just "-") in CHANGELOG.md and delete it or
replace it with a complete entry sentence describing the intended change; ensure
the file retains proper markdown list spacing and formatting after removal.
- Around line 21-29: Update CHANGELOG.md to document the new tenant quota
enforcement feature by replacing the incomplete "- " entry on line 30 with a
clear entry that mentions TenantQuotaEnforcer and QuotaExceededException and
summarizes enforced limits (per-tenant max FAQs, categories, users, max
attachment size, and API request rate limits). Mention where quota checks were
added (Faq, CategoryRepository, AbstractAttachment, User) so readers can find
related implementation points.
In `@phpmyfaq/src/phpMyFAQ/Tenant/TenantQuotaEnforcer.php`:
- Around line 59-77: The multiplication maxAttachmentSizeMb * 1024 * 1024 can
overflow on 32-bit PHP; update assertCanStoreAttachment in TenantQuotaEnforcer
to guard against overflow by checking if $maxAttachmentSizeMb exceeds
PHP_INT_MAX / (1024*1024) and, if so, set $maxSizeBytes to PHP_INT_MAX (or use a
safe float-based calculation and cast), then use that safe $maxSizeBytes for the
comparison with readAttachmentSizeBytes() and $newAttachmentSizeBytes before
throwing QuotaExceededException; reference the assertCanStoreAttachment method,
$maxAttachmentSizeMb, readAttachmentSizeBytes(), and QuotaExceededException so
the check is added in that function.
🧹 Nitpick comments (3)
phpmyfaq/src/phpMyFAQ/Tenant/TenantQuotaEnforcer.php (1)
79-107: TOCTOU race condition between quota check and insert.The count query and the subsequent insert (in the caller) are not atomic. Under concurrent requests, two callers could both pass the quota check and then both insert, exceeding the limit. This is an inherent limitation of application-level quota enforcement.
For most use cases this is acceptable, but if strict enforcement is required, consider a DB-level approach (e.g., a trigger or a serializable transaction wrapping the check + insert).
phpmyfaq/src/phpMyFAQ/Faq.php (1)
95-95: Minor: Missing docblock and blank-line separator for the new property.Other properties in this class have docblocks (e.g.,
$plurals,$user,$groups,$groupSupport). Adding one here would keep the style consistent.Proposed fix
private bool $groupSupport = false; + + /** + * Tenant quota enforcer. + */ private ?TenantQuotaEnforcer $tenantQuotaEnforcer = null;tests/phpMyFAQ/FaqTest.php (1)
54-61: Consider also clearing the cached enforcer in tearDown.After
testCreateThrowsWhenFaqQuotaIsExceeded, theFaqinstance retains a lazily-cachedTenantQuotaEnforcerholding the oldTenantContext(withmaxFaqs=0). This is safe today because a freshFaqis created in eachsetUp(), but if tests ever share instances, the stale enforcer could cause surprising failures. Clearing the env var (line 57) is sufficient for the current test structure.
thorsten
force-pushed
the
feat/tenant-quotas
branch
from
a60af56 to
3864635
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 23: Update the CHANGELOG entry line that reads "added experimental
per-tenant quota enforcement covering max FAQs, categories users, attachment
size, and API request rate limits (Thorsten)" to insert the missing comma
between "categories" and "users" so it reads "categories, users"; keep the rest
of the entry unchanged.
In `@phpmyfaq/src/phpMyFAQ/Tenant/TenantQuotaEnforcer.php`:
- Line 87: Replace calls to the global Database::getTablePrefix() with the
tenant-specific prefix from the injected TenantContext: use
$this->tenantContext->getTablePrefix() in the COUNT query construction in
TenantQuotaEnforcer (the occurrences around the COUNT query and the later
similar usage in the same class), and after making these replacements remove the
now-unused use phpMyFAQ\Database; import if it is no longer referenced elsewhere
in TenantQuotaEnforcer.
🧹 Nitpick comments (2)
phpmyfaq/src/phpMyFAQ/User.php (1)
138-139: Nit: Add a blank line before the new property to separate concerns.The
$authSourceand$tenantQuotaEnforcerproperties serve different purposes. A blank line would improve readability.private string $authSource = 'local'; + private ?TenantQuotaEnforcer $tenantQuotaEnforcer = null;phpmyfaq/src/phpMyFAQ/Attachment/AbstractAttachment.php (1)
267-270: Inconsistent visibility:protectedhere vsprivatein Faq, CategoryRepository, and User.The
getTenantQuotaEnforcer()method isprivatein the other three integration points butprotectedhere. If the intent is to allow test subclasses to override it, the same reasoning would apply to the other classes. Consider aligning toprivatefor consistency, or making all fourprotectedif override flexibility is desired.
1 participant
Summary by CodeRabbit
New Features
Tests