Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3230 closed enhancement (invalid)

<MODEL>.validate() doesn't consider unique=True in model definition.

Reported by: iaihmb@… Owned by: adrian
Component: Database layer (models, ORM) Version:
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I've read it's bad practice to wrap <MODEL>.save() in try and except blocks which means I've got to do something like this:

def userCreate(request):
    if request.method == 'POST':
        name, password = request.POST['name'], request.POST['password']
        sha1 = _sha1()
        sha1.update(password)
        password_hash = sha1.hexdigest()
        form = UserCreateForm({'name': name, 'password': password})
        if form.errors:
            return render_to_response('user_create.html', {'form': form})
        try:
            User.objects.get(name=name)
            error = "The name that you've chosen is already taken."
            return render_to_response('user_create.html', {'error': error, 'form': form})
        except User.DoesNotExist:
            user = User(name=name, password_hash=password_hash)
            user.save()
            return HttpResponseRedirect('/user/%i/' % user.id)
    return render_to_response('user_create.html')

Change History (9)

comment:1 Changed 8 years ago by Thomas Steinacher <tom@…>

See also #2129 which was closed as "wontfix".

comment:2 Changed 8 years ago by Michael Radziej <mir@…>

How is this related to <Model>.validate(), and what's the bug?

comment:3 Changed 8 years ago by Thomas Steinacher <tom@…>

I didn't test the bug, but I think that the validate() method of the model doesn't check whether the object with the given name already exists in the database. So he is checking manually whether it exists.

comment:4 follow-up: Changed 8 years ago by Michael Radziej <mir@…>

Yeah, and validate has not been fully implemented, yet, and should not be used at all.

But iaihmb doesn't even call validate() in his code, nor does newforms or Model.save(). I don't see where validate comes into play here. Shouldn't the summarybe 'newforms does not check uniqueness' or similar?

comment:5 in reply to: ↑ 4 Changed 8 years ago by Thomas Steinacher <tom@…>

Replying to Michael Radziej <mir@noris.de>:

But iaihmb doesn't even call validate() in his code, nor does newforms or Model.save(). I don't see where validate comes into play here. Shouldn't the summarybe 'newforms does not check uniqueness' or similar?

He doesn't call validate() because it wouldn't check the uniqueness of the object. So if validate() isn't implemented yet, I suggest closing this as wontfix.

I don't know whether it works with newforms or not – I didn't check that.

comment:6 follow-up: Changed 8 years ago by Michael Radziej <mir@…>

  • Resolution set to invalid
  • Status changed from new to closed

I don't understand the report, and the reporter doesn't reply.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by anonymous

Replying to Michael Radziej <mir@noris.de>:

I don't understand the report, and the reporter doesn't reply.

I haven't replied because I don't think that I could make the problem any clearer, Thomas Steinacher figured it out.

Replying to Michael Radziej <mir@noris.de>:

Yeah, and validate has not been fully implemented, yet, and should not be used at all.

But iaihmb doesn't even call validate() in his code, nor does newforms or Model.save(). I don't see where validate comes into play here. Shouldn't the summarybe 'newforms does not check uniqueness' or > similar?

<MODEL>.validate() will return true even if the model definition includes unique=True and the data isn't unique. As I said in the description the code I posted was a workaround for <MODEL>.validate() because it doesn't function as expected. I know that <MODEL>.validate() isn't complete, it was a suggestion for the implementation. The new forms library isn't complete either and people create tickets for it, so what's the problem?

Either way, in hindsight the suggestion was a bad one once you take into consideration the race condition.

comment:8 Changed 8 years ago by iaihmb@…

That was me (iaihmb@…). Sorry if I came off as dick, I was in a bit of a hurry.

comment:9 in reply to: ↑ 7 Changed 8 years ago by Thomas Steinacher <tom@…>

Either way, in hindsight the suggestion was a bad one once you take into consideration the race condition.

There are race conditions in other parts of Django, e.g. in get_or_create, (see my mailing list thread for more information)

Note: See TracTickets for help on using tickets.
Back to Top