Page MenuHomePhorge

Preliminary implementation of a greylisting database and spf service
AbandonedPublic

Authored by vanmeeuwen on Oct 23 2020, 4:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 5:34 AM
Unknown Object (File)
Fri, Nov 8, 7:52 AM
Unknown Object (File)
Sun, Nov 3, 9:03 PM
Unknown Object (File)
Sun, Oct 27, 8:09 AM
Unknown Object (File)
Sun, Oct 20, 7:00 AM
Unknown Object (File)
Oct 2 2024, 11:33 AM
Unknown Object (File)
Sep 16 2024, 8:14 PM
Unknown Object (File)
Sep 11 2024, 6:28 AM
Subscribers
Restricted Project

Diff Detail

Repository
rK kolab
Branch
dev/greylisting
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 31519
Build 12010: arc lint + arc unit

Event Timeline

vanmeeuwen created this revision.
  • Chiptune some of the tests and logic
machniak subscribed.
machniak added inline comments.
src/app/Console/Commands/User/GreylistCommand.php
63

In cases like this I find multiple where() calls looking better.

src/app/Greylist/Connect.php
32

This invokes a sql query. Checking recipient_type will be faster.

49

Same as above.

src/app/Greylist/Request.php
107

You don't need this query if $recipient is empty, right? Move it under the if below.

202

Looks like this query might be execute twice. Look above.

252

Cannot these be done in a single query?

297

I think these two can be replaced by ->where('updated_at', '>=', $this->timestamp->copy()->subDays(7)), with or without ->toDateTimeString(), I'm not sure.

304

sizeof -> count, for consistency.

src/app/Http/Controllers/API/V4/PolicyController.php
35

else not needed after return.

73

Calling has() and then get() looks like a redundant roundtrip.

135

You don't need call_user_func_array().

171

Similar method exists in Greylist/Request. This could be de-duplicated somehow, maybe?

src/app/Http/Kernel.php
43

I guess we should specify different limits for the PolicyController routes.

src/app/User.php
577

I'm not sure I like this functionality in the User model class.

src/app/Utils.php
241

Utils is not that big yet, but maybe we should have NetUtils.

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
45

The same columns like in the unique() below, so I think this is redundant.

50

This also will be redundant if you re-order columns in the unique index below.

84

There should be index here.

src/tests/Feature/Stories/GreylistTest.php
48

We have already getObjectProperty() helper.

src/tests/TestCaseTrait.php
92

this will be confused with existing assertEquals(), and I don't see why not just use assertSame().

178

I really don't like this extra newline. Our phpcs setup does not require this, maybe your editor?

344

This will be applied to every test. I don't like it. Maybe it should be in a separate trait, that you'd add to tests you really need to.

This revision now requires changes to proceed.Oct 26 2020, 2:13 PM
vanmeeuwen marked 14 inline comments as done.
  • Update with feedback

Some of the feedback is fixed.

src/app/Http/Controllers/API/V4/PolicyController.php
135

But... call_user_func()? Or what?

src/app/Http/Kernel.php
43

I had not found how to actually do that.

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
45

It is not, same as below.

50

A unique constraint may cause an index to be created, but it is a different type of index -- because it can't help you speed up queries by hitting the right index in the right way.

src/tests/TestCaseTrait.php
178

No, my reading habits do -- I read vertical space used much, much faster than horizontal space.

344

We can change it further when we have switched over most if not all tests to using volatile fixtures and not seeded contents, and split up multiple test functions per test class such that there's only one series of tests per class (currently all test functions assert the same start state).

  • Add an index for greylist_settings

Added the missing index.

src/app/Http/Controllers/API/V4/PolicyController.php
135

$object->senderPolicyFrameworkWhitelist($data['client_name']) ? ;)

src/app/Http/Kernel.php
43

I've found it could be done per route like this Route::group(['prefix' => 'api', 'middleware' => 'throttle:3,10'], function () { or probably just create a seaparate middleware label for these heavy APIs. https://laravel.com/docs/6.x/middleware#assigning-middleware-to-routes

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
45

They are the same.

$table->unique(
	        ['sender_local', 'sender_domain', 'recipient_hash', 'net_id', 'net_type'],
	         'ssrnn_unq'
 );
50

I'll trust you know better in this case, but I always thought that unique index is used as any other index. Why not?

src/tests/TestCaseTrait.php
344

In my eyes this creates a mess right now. Isn't that breaking any existing tests?

  • Unify clientNetFromAddress definitions to a single \App\Utils version
vanmeeuwen marked 2 inline comments as done.
  • Rename clientNetFromRequest to getNetFromAddress
vanmeeuwen added inline comments.
src/app/Http/Controllers/API/V4/PolicyController.php
135

Of course. I complete overlooked that.

src/app/Http/Kernel.php
43

I reckon too that our internal infrastructure is not supposed to be limited at this level, at all.

src/database/migrations/2020_10_18_091319_create_greylist_tables.php
50

A unique index is a constraint that incidentally uses an index in order to speed up the lookup to pass or fail on a unique constraint.

For example, a unique constraint could simply exist of a single column using a hash over cells appended to one another. It doesn't concern itself with comparison or range lookups, can completely ignore column types and is as such, when implemented correctly (so as to be most efficient as a unique constrained) in a database technology, it isn't an index you'd likely hit with a query so as to get to a resulting row.

src/tests/TestCaseTrait.php
344

Existing tests either use seeded content or set themselves up with a per-test domain/users/etc.

In dev/testing though, all tests are intended to use one single set of 'fixtures' -- however many of those there may be, and in however many "sets" we may break them down so as to make tests run more efficiently, and to whichever extent we may indeed refactor tests such that fixtures setup and teardown can happen once per class rather than once per test.

vanmeeuwen marked 2 inline comments as done.
  • Just call the method

Abandoned in favor of the combined D2434