-
Notifications
You must be signed in to change notification settings - Fork 45
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
Comments
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. |
Understood and same.
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 😢 |
@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? |
@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.) |
@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. |
In the mean time, I've put together a really hacky controller that can be used to implement a subset of the https://gist.github.com/Westie/9dbdc82468c25dc4567e6d89f75036d7 For clarity, this is just essentially just a mapper for the actions traits |
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 appreciate your afford towards making this beautiful package. |
I was thinking about this. I realize it's not an efficient way to solve the problem, but, couldn't the following be possible:
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. |
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). |
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. |
@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. |
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. |
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! |
@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. |
@lindyhopchris Hello, is there a branch accessible for us to review the implementation? We're eager to contribute and collaborate towards getting this operational. |
So on the lower-level packages, the On the main package (this one), there's a working branch here: 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);
}
} |
Any further thoughts on first-party documentation solution? |
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? |
@glennjacobs primarily curiosity or would you have OSS time to help push things along? |
@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? |
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: |
@lindyhopchris looks like that project isn't visible publicly at the moment? |
thanks, should be public now. |
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!
The text was updated successfully, but these errors were encountered: