Coder Social home page Coder Social logo

Comments (3)

saxix avatar saxix commented on May 19, 2024

should not be difficult to implement, I do not have time to make proper tests, anyway take a look at concurrency.core._select_lock() at line 44. value should be None (or 0 if first save).
It's possible to create a flag to enable this check to do not create backward incompatibility. I like the idea and I see the benefits, anyway do not think I have time now to work on that. If you want to create a proper pull request I'll be happy to merge it.

from django-concurrency.

claytondaley avatar claytondaley commented on May 19, 2024

+1

Just ran into this issue. A pre-concurrency UI was sending REST requests without a version; no errors were generated and data was overwritten. It seems like a pretty serious bug if you can defeat the entire system through omission.

This strikes me as so serious that I'd recommend a major version change to make the opposite behavior "default". If you insist on persisting a very dangerous condition in the name of backwards compatibility, maybe a setting to override the default?

from django-concurrency.

claytondaley avatar claytondaley commented on May 19, 2024

Turns out I had an issue at two different levels:

  1. Django REST Framework (DRF) was not requiring the version (even though it was required in the model) and then was "filling it in" with the value from the database. Obviously, this broke the concurrency check.

I've submitted a ticket at DRF and worked around this issue in my local copy. The logical fix from the perspective of the REST framework was to, at minimum, use the default value (if available) for any value absent in the PUT. The default for these fields is version=0. Unfortunately, that leads to the second issue:

  1. django-concurrency sees the default value and skips version checks here

I'm about to submit a PR that:

  • Creates a settings flag IGNORE_DEFAULT=True
  • Adapts the field logic to check the version unless this flag is enabled

I also had to account for an edge case (documented inline) that can occur during a create. This required a change in the way the logic flowed.

I added a test that failed under the old code. All of the old tests pass except one. As I'll reiterate in the PR, I'm not clear why the test should fail to fix the code. If you can explain, I'll update the PR accordingly.

from django-concurrency.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.