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

Will atomic operations be supported here long-term? #39

Open
acorncom opened this issue Feb 24, 2021 · 25 comments
Open

Will atomic operations be supported here long-term? #39

acorncom opened this issue Feb 24, 2021 · 25 comments

Comments

@acorncom
Copy link

Hey @lindyhopchris, appreciate all your hard work on this (and the previous version as well). Was curious if the atomic operations extension to the spec (https://jsonapi.org/ext/atomic/) is on the radar as this gets built out? Poked around a bit and wasn't seeing anything mentioned, so thought I'd ask. I'm contemplating a long-term stack migration a client may need so thinking through our options there.

Thanks!

@lindyhopchris
Copy link
Contributor

Hi! Thanks for flagging that up, I hadn't realised that they'd actually published that as an extension.

So the short answer is yes, because I have production APIs where that functionality would be exceptionally useful.

The longer answer is it slightly fills me with dread because I've just spent a considerable amount of time sorting out the new package, and issues are already starting to build up! I'm going to need help going forward to add significant functionality like that extension, so my question for you would be - if it's important for you to add, is it something you'd be able to collaborate on?

The only problem I foresee with it, is I've just moved all the validation to Laravel form requests, to make things more Laravel-y, which is a change I wanted to make because some of the feedback on the previous version was that it had too steep a learning curve.

The problem with those operations is we'd need to validate each operation in turn... which means access to the validation rules for a resource. Which are currently in a form request, which Laravel automatically validates when resolving from the container. So we'd probably have to extract the validators to a separate class again. Which feels like I've wasted a load of time adopting form request, d'oh.

@acorncom
Copy link
Author

acorncom commented Feb 25, 2021

So the short answer is yes, because I have production APIs where that functionality would be exceptionally useful.

Understood and same.

The longer answer is it slightly fills me with dread because I've just spent a considerable amount of time sorting out the new package, and issues are already starting to build up! I'm going to need help going forward to add significant functionality like that extension, so my question for you would be - if it's important for you to add, is it something you'd be able to collaborate on?

Fully understood on it filling you with dread, it's going to take some work to revamp things to account for the new wrinkles that adds. As far as something I'd be able to collaborate on, I would very much like to. Sadly, although I see needs for it on various client projects, I don't have a burning need for it and things are pretty busy at the moment, which means I'm afraid I don't have the time to give this the focused attention it would take 😢

@lindyhopchris
Copy link
Contributor

@acorncom no problem. I can definitely commit to doing atomic operations at some point in the future, just can't guarantee when.

Out of interest, you mentioned when opening the issue that you're thinking through options. What other options are you looking at?

@BenWalters
Copy link

@lindyhopchris I know you've stated that you cannot guarantee when you might be able to deliver atomic operations, but figured I'd ask since your last post was back in Feb. Is this on your radar/likely to be delivered in the foreseeable?

We've got a situation where I think atomic operations is the answer. I probably have a solution but it has drawbacks that I'd rather avoid (goes against spec/naming conventions are broken, etc.)
Thanks!

@lindyhopchris
Copy link
Contributor

@BenWalters thanks for your question. I'm also keen to use atomic operations. However, as my open source time is pretty limited at the moment, I'm only planning to implement them once they have definitely been added to the spec - i.e. JSON:API 1.1 is tagged.

Basically I don't want to spend the time dev'ing it for it to then be modified before being tagged. Even if I dev it, I wouldn't want to tag this package with it until it was tagged in the spec. Hope that makes sense!

FYI it will be quite a big refactoring. This is because I went with Laravel form requests when implementing this package, as I wanted to use standard Laravel conventions as much as possible. The problem is that couples create and update operations to the HTTP request. So basically, I'm going to have to separate that out to implement atomic operations.

I'll be keen to do the work once they tag JSON:API 1.1, but I just can't afford to spend the time doing it when I don't know when they are going to tag atomic operations.

@Westie
Copy link

Westie commented Apr 20, 2022

In the mean time, I've put together a really hacky controller that can be used to implement a subset of the atomic:operations spec.

https://gist.github.com/Westie/9dbdc82468c25dc4567e6d89f75036d7

For clarity, this is just essentially just a mapper for the actions traits

@lindyhopchris
Copy link
Contributor

lindyhopchris commented Jan 28, 2023

Update on this: JSON:API v1.1 has now been tagged, with the Atomic Operations extension in it.

It is a priority for me to add this to Laravel JSON:API - I need to use it in production applications as much as everyone else here who is keen to use it! Plus also "How do I do multiple operations in a single request?" is probably the most frequently asked question on Slack!

I need to be honest about my time at the moment - that really is the limiting factor! I've moved to a job in an organisation that doesn't use JSON:API, so I have no work time on this package. So anything that needs doing is entirely in my free time (though will still happen as my personal projects all use JSON:API).

Atomic Operations is a big refactor - because of the current coupling of the implementation to Laravel form requests.

So TLDR: I will get to this, please be patient and understanding 🙏

@lindyhopchris lindyhopchris pinned this issue Jan 28, 2023
@atapatel
Copy link

@lindyhopchris appreciate your afford towards making this beautiful package.

@DellanX
Copy link

DellanX commented Feb 5, 2024

I was thinking about this. I realize it's not an efficient way to solve the problem, but, couldn't the following be possible:

  1. When an atomic request is received, only parse the top level schema (Just the JSONAPI schema)
  2. Divide the request into sub-requests for each atomic operation
  3. Fire off each request (From Laravel to Laravel), to which Laravel could process the individual request's Schema correctly
  4. Stitch the responses together
  5. return the overall response.

Again, definitely not an efficient way to solve this problem, but I think it could work.

  "atomic:operations": [{
    "op": "update",
    "data": {
      "type": "articles",
      "id": "13",
      "attributes": {
        "title": "To TDD or Not"
      }
    }
  }]

becomes:

[
  "atomic:operations":  array,
  "atomic:operations.*.op": "string",
  "atomic:operations.*.data.type": "string",
  "atomic:operations.*.data.id": "sometimes|string",
  "atomic:operations.*.data.attributes": "array", // We can't apply any rules here yet
]

Then, when the internal request is fired off from Laravel to Laravel, attributes can be compared against the schema for that operation.

@lindyhopchris
Copy link
Contributor

I would never fire a request from a Laravel app to the same Laravel app. It's an indication of bad design - i.e. indicates code is too closely coupled to an HTTP request, which is the problem with have here due to the use of Laravel form requests.

I have actually implemented most of the code changes required, I just need to finish a few things, then package them up as an alpha release. The big remaining thing to do is update the docs and write an upgrade guide (which is required as no one will be able to do the upgrade without a guide).

@lindyhopchris
Copy link
Contributor

I appreciate this is taking time, but it's a huge change with limited open source time - so just need some patience, but it's definitely coming.

@lindyhopchris
Copy link
Contributor

@DellanX also worth mentioning the other reason we just can't send a Laravel request for each operation is the Atomic Operation extension specifies that all the operations must be atomic... i.e. if one fails, all of them fail. I.e. they are transactional across all of them. That couldn't be guaranteed if you were sending an HTTP request within the app for each operation in the original request.

@DellanX
Copy link

DellanX commented Feb 5, 2024

To clarify, I don't have any need for atomics at this time. Your repo has made life for me a thousand times easier, so I was hoping to give back, and thought it could be a much simpler workaround rather than refactoring the package.

I do agree, HTTP requests is an awful way to implement this, and as you said, it wouldn't be able to achieve the specification.

@lindyhopchris
Copy link
Contributor

ah no problem, thanks for the suggestion - was just explaining why refactoring is a better option from my perspective.

I've going to sort out Laravel 11 support on the current version later this week, then look at the final admin for the first alpha release after that, so this has now got back to the top of my todo list!

@defser
Copy link

defser commented Apr 12, 2024

@lindyhopchris Thank you for the update regarding the development of this feature. We would love to be part of the Alpha testing of the Atomic Operations extension.

Please let me know when we can start using Atomic Operations extension.

@defser
Copy link

defser commented May 7, 2024

@lindyhopchris Hello, is there a branch accessible for us to review the implementation? We're eager to contribute and collaborate towards getting this operational.

@lindyhopchris
Copy link
Contributor

So on the lower-level packages, the next branch contain the changes for the next major release.

On the main package (this one), there's a working branch here:
develop...feature/core-v5

That's not a stable branch, I'm using it just to prove things are working (or not) and track down bugs. Not all the tests are passing, which working through the failing tests is my priority.

It hasn't got Atomic Operations in it yet. However it has 100% of the changes required to unblock Atomic Operations. So my plan is to fix the remaining tests - the tests are right, the failures indicate regressions that I need to fix. Then I'd be ready for an alpha release (will need to write an upgrade guide). Then I drop in Atomic Operations.

The main change is I've had to remove all the Form Requests so that we're decoupled from HTTP requests. That unlocks Atomic Operations, where we'd need to run one-to-many JSON:API operations (whereas previously an operation was tied directly to a Form Request).

This has been a very significant rewrite, which is why this is taking long (made worse by limited Open Source time, though I'm starting to pick stuff back up again now).

One of the really nice benefits is that validation is now on the fields in the schema which works a treat. I'm also introducing PHP attributes to the schemas. Both changes for me are 🎉

Here's a flavour of the new world:

namespace App\JsonApi\V1\Posts;

use App\Models\Post;
use Illuminate\Http\Request;
use Illuminate\Validation\Rule;
use LaravelJsonApi\Core\Schema\Attributes\Model;
use LaravelJsonApi\Eloquent\Fields\DateTime;
use LaravelJsonApi\Eloquent\Fields\ID;
use LaravelJsonApi\Eloquent\Fields\Relations\BelongsTo;
use LaravelJsonApi\Eloquent\Fields\Relations\BelongsToMany;
use LaravelJsonApi\Eloquent\Fields\Relations\HasMany;
use LaravelJsonApi\Eloquent\Fields\Relations\MorphToMany;
use LaravelJsonApi\Eloquent\Fields\SoftDelete;
use LaravelJsonApi\Eloquent\Fields\Str;
use LaravelJsonApi\Eloquent\Filters\OnlyTrashed;
use LaravelJsonApi\Eloquent\Filters\Scope;
use LaravelJsonApi\Eloquent\Filters\Where;
use LaravelJsonApi\Eloquent\Filters\WhereIdIn;
use LaravelJsonApi\Eloquent\Pagination\PagePagination;
use LaravelJsonApi\Eloquent\Schema;
use LaravelJsonApi\Eloquent\SoftDeletes;

#[Model(Post::class)]
class PostSchema extends Schema
{
    use SoftDeletes;

    /**
     * @inheritDoc
     */
    public function fields(): array
    {
        return [
            ID::make(),
            BelongsTo::make('author')->type('users')->readOnly(),
            HasMany::make('comments')->canCount()->readOnly(),
            Str::make('content')->rules('required'),
            DateTime::make('createdAt')->sortable()->readOnly(),
            SoftDelete::make('deletedAt')->sortable(),
            MorphToMany::make('media', [
                BelongsToMany::make('images'),
                BelongsToMany::make('videos'),
            ])->canCount(),
            DateTime::make('publishedAt')->sortable(),
            Str::make('slug')
                ->rules('required')
                ->creationRules(Rule::unique('posts'))
                ->updateRules(fn($r, Post $model) => Rule::unique('posts')->ignore($model)),
            Str::make('synopsis')->rules('required'),
            BelongsToMany::make('tags')->canCount()->mustValidate(),
            Str::make('title')->sortable()->rules('required'),
            DateTime::make('updatedAt')->sortable()->readOnly(),
        ];
    }

    /**
     * @inheritDoc
     */
    public function filters(): array
    {
        return [
            WhereIdIn::make($this)->delimiter(',')->onlyToMany(),
            Scope::make('published', 'wherePublished')->asBoolean(),
            Where::make('slug')->singular()->rules('string'),
            OnlyTrashed::make('trashed'),
        ];
    }

    /**
     * @inheritDoc
     */
    public function pagination(): PagePagination
    {
        return PagePagination::make()
            ->withoutNestedMeta()
            ->withMaxPerPage(200);
    }
}

@glennjacobs
Copy link

Any further thoughts on first-party documentation solution?

@lindyhopchris
Copy link
Contributor

Yes it's still the plan to add that, but it is further down the track. I can't put a date on it yet.

@glennjacobs
Copy link

Yes it's still the plan to add that, but it is further down the track. I can't put a date on it yet.

Any idea on how you are thinking of approaching it?

@acorncom
Copy link
Author

acorncom commented May 9, 2024

@glennjacobs primarily curiosity or would you have OSS time to help push things along?

@acorncom
Copy link
Author

acorncom commented May 9, 2024

@lindyhopchris the above looks great. is helping sort through failing tests / regressions something you want help with if those of us interested have time or would that slow you down?

@lindyhopchris
Copy link
Contributor

I think that would slow me down.

I'm starting to pick things back up, though I'm currently dealing with the backlog of things people have raised with the current version. Particularly as some people have been kind enough to submit PRs, so want to ensure the code they've taken the time putting together actually gets out.

To start to put more transparency around what's going on, and just how complex and big things are, I've created a Project Kanban board. I'll be filling it out with the things I need to do for this next major version, but also pulling in anything I want to prioritise on the current version. Here's the link:
Big Board of Everything

@acorncom
Copy link
Author

@lindyhopchris looks like that project isn't visible publicly at the moment?

@lindyhopchris
Copy link
Contributor

thanks, should be public now.

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

No branches or pull requests

8 participants