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.

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

Automatic auditing of attach/detach for BelongsToMany #869

Open
gisostallenberg opened this issue Oct 6, 2023 · 7 comments
Open

Automatic auditing of attach/detach for BelongsToMany #869

gisostallenberg opened this issue Oct 6, 2023 · 7 comments

Comments

@gisostallenberg
Copy link
Contributor

Q A
Bug? no
New Feature? yes
Framework Laravel
Framework version 10
Package version 13
PHP version 8.1

To audit attach and detach at the moment you'd have to call Auditable::auditAttach or Auditable::auditDetach. I have a solution to audit the attach/detach while the normal methods are called.
Would you be interested into a PR with this solution?

It works as follows:

<?php

declare(strict_types=1);

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use OwenIt\Auditing\Auditable;
use OwenIt\Auditing\Contracts\Auditable as AuditableContract;
use OwenIt\Auditing\Database\Relations\BelongsToManyAudit;

final class MyModel extends Model implements AuditableContract
{
    use Auditable;

    public function otherModels(): BelongsToMany
    {
        return BelongsToManyAudit::audit($this->belongsToMany(OtherModel::class));
    }

}
<?php

declare(strict_types=1);

namespace OwenIt\Auditing\Database\Relations;

use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use Illuminate\Support\Facades\Event;
use OwenIt\Auditing\Contracts\Auditable;
use OwenIt\Auditing\Events\AuditCustom;

/**
 * @property Model&Auditable $parent
 *
 * @method Collection getResults()
 *
 * @mixin BelongsToMany
 */
final class BelongsToManyAudit extends BelongsToMany
{
    protected function __construct()
    {
        parent::__construct(...\func_get_args());
    }

    public static function audit(BelongsToMany $belongsToMany): static
    {
        return new self(
            $belongsToMany->related->newQueryWithoutRelationships(),
            $belongsToMany->parent,
            $belongsToMany->table,
            $belongsToMany->foreignPivotKey,
            $belongsToMany->relatedPivotKey,
            $belongsToMany->parentKey,
            $belongsToMany->relatedKey,
            $belongsToMany->relationName,
        );
    }

    /**
     * Audit attach to the parent of the relation.
     *
     * {@inheritdoc}
     *
     * @param array<string, mixed> $attributes
     */
    public function attach($id, array $attributes = [], $touch = true): void
    {
        $old = $this->getResults()->toArray();

        parent::attach($id, $attributes, $touch);

        $this->auditEvent('attached', $old, $this->getResults()->toArray());
    }

    /**
     * Audit detach from the parent of the relation.
     *
     * {@inheritdoc}
     */
    public function detach($ids = null, $touch = true): int
    {
        $old = $this->getResults()->toArray();

        $result = parent::detach($ids, $touch);

        $this->auditEvent('detached', $old, $this->getResults()->toArray());

        return $result;
    }

    /**
     * @param array<string, mixed> $old
     * @param array<string, mixed> $new
     */
    private function auditEvent(string $eventName, array $old, array $new): void
    {
        /** @var Auditable $model */
        $model = clone $this->parent;

        if (!\property_exists($model, 'auditEvent') || !\property_exists($model, 'isCustomEvent') || !\property_exists($model, 'auditCustomOld') || !\property_exists($model, 'auditCustomNew')) {
            return;
        }
        $model->auditEvent = $eventName;
        $model->isCustomEvent = true;
        $model->auditCustomOld = [$this->relationName => $old];
        $model->auditCustomNew = [$this->relationName => $new];

        Event::dispatch(AuditCustom::class, [$model]);
    }
}
@parallels999
Copy link

parallels999 commented Oct 6, 2023

And MorphToMany? sync()?

It would also be a breaking change, because everyone should already be using auditSync, and not to mention that if someone is already using a custom relationship this would already be a conflict,
also, it's not so "automatic"

@gisostallenberg
Copy link
Contributor Author

also, it's not so "automatic"

What do you mean? I meant: If you want to audit specific relationship attach/detachments, you can implement it in the main point where your intention to audit lies, not at various points in your existing codebase. In the documented manner you'd have to change all calls from attach to auditAttach.
Not to mention this case, which is not auditing in the documented manner:

        $model = MyModel::factory()
            ->hasAttached($otherModel)
            ->create();

In the proposed solution this will get audited. I call that automated.
It is only 'automatic' when someone decorates the BelongsToMany, so there is a upgrade path. It is not breaking in any way. Just leave your codebase as is and nothing will happen...

I think I forgot sync, which can be simply added.

I'd have to check if MorphToMany can be an argument to BelongsToManyAudit::audit. That is one of the reasons I opened an issue before creating a PR.

@parallels999 thanks for your cheerfulness about this issue 😉

@angeljqv
Copy link
Contributor

angeljqv commented Oct 6, 2023

dcb1a6b, 43fbbe6

I think it's a great idea, but you could no longer decide if you want to audit only the id column
maybe adding some property that indicates which columns should be audited in the relationship

return BelongsToManyAudit::audit($this->belongsToMany(OtherModel::class), [
    'audited_column_1', 'audited_column_2'
]);

@MortenDHansen
Copy link
Contributor

I see the idea. Might need to think a bit though.

At first i was reluctant to make the relations audit because the whole concept is basically to audit by the events (audit by model). Using the custom audit is a way to get the job done when you attach a role to a user (or similar), and the supplied methods are basically just a documented workaround for the missing events.

The manual effort of explicitly using the auditSync/attach/detach front methods are actually quite intentional so that it is only used deliberately.

@parallels999
Copy link

Also #873 would be needed

@nikspyratos
Copy link

nikspyratos commented Feb 7, 2024

To audit attach and detach at the moment you'd have to call Auditable::auditAttach or Auditable::auditDetach

Side note, aren't these only for morphs right now? The only doc I find on these functions is in relation to morphs

@gisostallenberg
Copy link
Contributor Author

@nikspyratos MorphToMany extends BelongsToMany and these are also to audit a BelongsToMany relation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants