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

Implement form validation & redirect back with errors #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

antoniosarosi
Copy link

@antoniosarosi antoniosarosi commented Nov 11, 2023

Following up on #30, here's a basic implementation of Inertia Validation. Quoting the docs:

If there are server-side validation errors, you don't return those errors as a 422 JSON response. Instead, you redirect (server-side) the user back to the form page they were previously on, flashing the validation errors in the session.

In order for your server-side validation errors to be available client-side, your server-side framework must share them via the errors prop.

That's what the code does, when not form.is_valid() you can raise InertiaValidationError and this library handles flashing validation errors in the session, redirecting and passing the errors prop to Inertia. For now you must explicitly provide the "previous" URL where to user should be redirected, since it's not stored anywhere.

Basic usage example:

# views.py

import json
from inertia import render, InertiaValidationError
from django.shortcuts import redirect

class ExampleFormView(View):
    def get(self, request):
        return render(request, "SomeForm")

    def post(self, request):
        form = SomeForm(json.loads(request.body))

        if not form.is_valid():
            raise InertiaValidationError(form.errors, redirect("form"))

        # use form.cleaned_data here...

        return redirect("some_other_route")

# urls.py

urlpatterns = [path("/form", ExampleFormView.as_view(), name="form")]

Instead of raising the error manually you can also call the inertia_validate helper, which will raise the error automatically:

form = SomeForm(json.loads(request.body))
cleaned_data = inertia_validate(form, redirect("form"))

In some cases you don't actually know where the user should be redirected because the form might be located in a pop-up that you can trigger from multiple places or something similar. For example, imagine a "Login" button in the navbar that opens a side panel with a login form. For now, the best option is to use the HTTP Referer header to handle such cases:

if not form.is_valid():
    raise InertiaValidationError(form.errors, redirect(request.headers["Referer"])

In order to avoid redirecting manually we could change the API so that you pass the request and an optional fallback:

if not form.is_valid():
    raise InertiaValidationError(request, form.errors, fallback=redirect("some_route"))

This would try to redirect to request.headers["Referer"] if present, otherwise use the fallback, which could be set to "/" by default.

Technically, the Referer header is optional so we shouldn't rely 100% on it, Laravel for example stores the "previous" URL in the session on every GET request to its server. Here's some pseudocode based on Laravel 10.x implementation:

if (
    request.method == "GET"
    and not request.headers.get("X-Requested-With") == "XMLHttpRequest"
):
    request.session["_previous_url"] = request.build_absolute_uri()

But I don't think implementing this is necessary, because in practice all major browsers will send the Referer header and even Laravel favors Referer over everything else.

@antoniosarosi
Copy link
Author

antoniosarosi commented Nov 11, 2023

Some other notes:

Type Hints

This codebase doesn't use type hints internally but I think they should be used at least for the public API so that users can see the parameter types in their IDE, that's why I used type hints for the validation error and helper. For all the rest of things I tried following the same style that was already present.

Error messages

When Django validates a form it creates a list of error messages for each field, something like this:

{ "name": ["This field is required.", "Some other error"] }

To avoid having to write form.errors.name[0] in the frontend, the middleware automatically removes the list and preserves only the first error found:

{ "name": "This field is required." }

The official Laravel adapter does the same.

Non-field errors

Django has something called non_field_errors() which are not associated with any particular field. These errors are found in __all__ key, so you might have to do something like this in the frontend:

form.errors["__all__"]

Redirecting

If the form GET and POST URL is exactly the same, you probably don't need to redirect at all, you can render the component again passing errors manually:

def view(request):
    if request.method == "GET":
        return render(request, "Component")

    if request.method == "POST":
        form = SomeForm(json.loads(request.body))
        if not form.is_valid():
            return render(request, "Component", props={"errors": form.errors})
    # ...

I didn't check this but I think it works.

JSON

As per issue #21, we could provide some built-in functionality to simplify this:

form = SomeForm(json.loads(request.body))

@antoniosarosi
Copy link
Author

Just got another idea regarding this:

In order to avoid redirecting manually we could change the API so that you pass the request and an optional fallback:

if not form.is_valid():
    raise InertiaValidationError(request, form.errors, fallback=redirect("some_route"))

This would try to redirect to request.headers["Referer"] if present, otherwise use the fallback, which could be set to "/" by default.

Since middlewares run top to bottom and then bottom to top again, the Inertia middleware is always going to run before the user's view function. So it could store the Referer header or a reference to the entire request before calling get_response(request), and if the user raises InertiaValidationError we already have the request, no need to pass it as a parameter:

if not form.is_valid():
    raise InertiaValidationError(form.errors, fallback=redirect("some_route"))

Since fallback is also optional, the highest level API could look like this:

cleaned_data = inertia_validate(form)

I'll wait on some feedback before writing more code, but I think this looks pretty clean, it doesn't really do that much magic except for raising an error.

@svengt
Copy link
Contributor

svengt commented Nov 12, 2023

Hey @antoniosarosi,

Just curious about the specific use case you're tackling here. Like you mentioned, Django apps usually rerender the form after validation fails, and that's pretty standard for all the generic views. Are you dealing with a view that's just for POST requests?

I'm thinking that might be outside what this project is all about. Usually, just adding the error messages in the context sorts things out for Inertia compatibility. We also tweaked the Inertia implementation a bit in our projects to be compatible with Django's helper function f.errors.as_json().

My take is that it's probably better to tweak Inertia to fit into Django's way of doing things, not the other way around. This way, you get to make the most of what Django offers, especially with its generic class-based views. What do you think? Any specific challenges or reasons why a different approach might work better in your case?

@antoniosarosi
Copy link
Author

@svengt In my particular use case there's a login form that you can submit from any page available to non-authenticated users. There's a login button in the top navbar which instead of redirecting you to the login page opens up a sidebar with the form, and you submit it from wherever you are. When using Django templates you are pretty much forced to redirect the user to the login page or use Javascript to send the request from the current page, but with Vue or React you can take different approaches.

The general use case is validating a form that you don't know which page it's coming from or the GET and POST/PUT/PATCH URLs are not the same. Maybe you have a model that can be created or updated from multiple pages because it's related to such pages in some way, and instead of being redirected to a different page you can open a modal, submit the form from where you are and then continue what you were doing.

As I mentioned before, if you are using a frontend framework like Vue or React you are not as constrained as when you're using template rendering, so you can improve UX by doing things like this, there's less redirecting in general. And since you can't simply render the page again because you don't know which one to render, you need some solution. In the current state, either use JSON requests/responses or always redirect to specific CRUD pages like you would with templates. Both require more code and are not compatible with Inertia's way of handling form submissions.

I agree that this might not be exactly the Django way of doing things, but sticking 100% to how Django handles templates seems to limit what you can do with Inertia or require you to implement custom solutions. For example, Django doesn't have a built-in way to parse JSON requests as mentioned in #21, Django doesn't automatically serialize models/queries into JSON either and that's why InertiaJsonEncoder was implemented and further discussed in #18, and then there's this validation issue, and I'm sure people will find more.

On the other hand, raising an error to send a response is not unseen in Django, you can raise Http404 for example, so this doesn't really deviate so much from Django except for the "flashing errors into session" thing which is not common, there are built-in mechanisms for flashing messages but not errors. And I also proposed 3 different APIs from which we could choose the one that's more "Django-like", probably the one that requires passing the request manually.

Anyway, I'm not trying to convince you that this is a good idea, my initial thought was actually implementing this in a separate middleware so that you can opt-in by placing it into settings.py. I implemented it in the current built-in middleware because it was suggested by @BrandonShar, but even so it doesn't do anything unless you use the feature, so it's pretty much opt-in.

If this doesn't end up being a built-in functionality it could go into a separate Python package, probably more people who don't mind not being 100% Django compatible would find it useful. I suppose it's not necessary for simple apps where you can always re-render on the spot but I think it comes in handy for medium size apps that might get a little more complex on the frontend side.

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

Successfully merging this pull request may close these issues.

2 participants