fix problem with long Url.pattern from model_bakery and admin

[PetrDlouhy]

fix #132

• The MaxLengthValidator has problem, that it doesn't allow / with 254 other characters (which is generated when saved previously with long pattern). But I suppose it is very minor problem not worth adding more complexity to the code.

[codecov]

Codecov Report

Merging #133 (8e5d9d9) into master (962103e) will increase coverage by 1.77%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   87.21%   88.99%   +1.77%     
==========================================
  Files          12       13       +1     
  Lines         305      327      +22     
  Branches       29       30       +1     
==========================================
+ Hits          266      291      +25     
+ Misses         28       26       -2     
+ Partials       11       10       -1     

Files Changed	Coverage Δ
src/robots/migrations/0003_alter_url_pattern.py	100.00% <100.00%> (ø)
src/robots/models.py	90.90% <100.00%> (+9.65%)	⬆️
src/robots/tests.py	94.51% <100.00%> (+0.59%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

[tony]

Thank you for this!

At this time, fixing #124 is a more urgent issue. I believe that will need to get resolved first.

I also note there's no test - which we probably want - but not having written one is understandable given the project's current state.

[tony]

I've created #136 to improve test and development infrastructure.

I will also document the test flow

[tony]

I haven't merged anything yet, but as-is (5.0 at 77b980c), here's how tests can be ran:

env PYTHONPATH=. DJANGO_SETTINGS_MODULE=tests.settings django-admin test robots -v2

[PetrDlouhy]

I have added test for this Issue and few other Url model tests.

Although the test would raise django.db.utils.DataError: value too long for type character varying(255) only on PostgreSQL testing database. SQLite is OK with longer than expected strings.

[tony]

@PetrDlouhy Sorry for the delay.

Are you still interested in this PR? Can you rebase this?

[PetrDlouhy]

@tony I rebased this.

[tony]

Q: Is this default="" necessary?

I am not sure, but couldn't this lock tables / cause potential downtimes on production sites?

[PetrDlouhy]

@tony I don't rememeber why I added the default="". I got rid of it and it seems, that tests are passing, so I would assume it is not necessary.

[tony]

@PetrDlouhy First, thank you for your time. ☺️

In regards to the description in the PR and in #132. Correct me if I'm mistaken below!

To summarize this PR: it adds a validator to solve a compatibility issue arising with a third party django package: model_bakery. Does this sound accurate?

Does this problem still effect you? e.g. Do you still run model_bakery and django-robots and run into this error? Sorry time has passed since then.

If this issue still happens for you: Is the PR still need needed now that blank='' was removed in 8e5d9d9? How do we know it's a bug with django-robots and not resolvable via model_bakery?

FYI: I haven't used model bakery FYI, so things are not obvious to me.

Thank you!

[PetrDlouhy]

@tony I don't think this is an issue only for model_bakery, but in that usage it becomes most prominent.

The problem is, that if you write 255 characters into the URL.pattern field it will end up in exception. The reason is that, the save() method will add the leading slash (/) character making the string 256 characters long.
The error does arise as well when you do this in django-admin. Instead of convenient validation error the user would get server error 500.

The problem with model_bakery is that it would crate the pattern string always 255 characters long, so that this error will arise anytime I would like to generate an URL.

And yes, I am still using model_bakery for this.

[tony]

@PetrDlouhy I am grateful for the pull request, thank you. Sorry for the delay!

I am concerned about merging in something that'd potentially have unintended side effects for users.

Questions

While the underlying change makes sense to me, no other user has brought up a concern yet since #132 was made. To help reframe this toward django / robots usage in general:

1. Are there any generic use cases to this change beyond third party model bakery package?

2. Is there a way in model bakery to workaround this issue?

3. In django-robots, is there an alternative approach to accomplish this without changing the model and introducing migration?

   • If so, what makes this the best choice (in your perspective)?

   • What do we have to assure us this won't break other usages downstream?

     As an example:

     • we have our existing tests and the tests you added in the PR
     • this usage is considered canonical django, e.g. django model validators and such are established practice.
     • if merged, we can release the change a pre-release (though I'm not sure if those are in fact used downstream)

Second opinion?

Can you please seek a second opinion from someone on Jazzband? The reason why is they would have an opinion that could bring insight and balance out mine.

Thank you again!

Let me know what you think on both of the above!

[PetrDlouhy]

@tony The migration is created by Django and is only required because the model state changes but doesn't make any real data change in the DB.
I could not create the migration and everything will work without problems, only the migration checks would throw exception.

I really don't understand your concerns. What exactly could go wrong with this change? What are the unintended side effects?

My use case is that I am using django-admin-smoke-tests for testing if everything works OK in the admin environment.

The problem couldn't be fixed in model bakery, because the error is in django-robots.

Please go to the Robots URL admin, click on "Add URL" button and fill in long string like sytowQXhbktgUxdOJHmFxhaLqzxWcsdfsdfsdfsdfsdfsdfqXNbafrVMXCLOjaRELXYSnEzgSgsqzsdfsdfsdfsdfsdfsdfsdfNTqJLkOZPCRvwrqbLtDQmzYghQAaVviHmjoMXRGvXpzJCxNcyfUpJHpaHiBGjyBbdNULZIHGuGUuiyIPkHnTJHNxoVEoDurWjeLgTeqLgSeeZcuzyRkVeNnunAhcaLbOOUwPlXSqqtHORtBLpWIabHLSTcJDIf and click save.
You will see exception, which is never good.
Users should always see nice pleasant failed validation like this:

Not this:

[PetrDlouhy]

And yes, model validators are exactly the right tool for this in Django. If the model is not able to accept 255 characters, there should be validator that ensure that users will not put 255 characters there.

[tony]

@PetrDlouhy

This looks sensible to me! Pardon the delay.

Let's give this a shot! Merged via 6dbeb31.

P.S. Why the delay? When maintaining, I have to take responsibility for what is merged in: If this affects downstream users in a way that wasn't anticipated, that can inconvenience them. The reason why is an earlier issue happened with migrations in #124.

[tony]

@PetrDlouhy

Released 7.0b0 (Release, PyPI, Tag)

If you give 7.0b0 a shot, how is that?

[PetrDlouhy]

@tony Thank you very much.
I don't know why yet, but tests on my project with 7.0b0 started to fail with value too long for type character varying(255).
I will have to look at it more closely what makes the difference between my branch and 7.0b0

[tony]

@PetrDlouhy Thanks! Look forward to you look into this further! You may be in a good position to if you still have model_bakery.

[PetrDlouhy]

@tony I realized, that the tests are not passing because of the removed default="" which was in fact fixing model_bakery .
I could re-add that and also add some model_bakery tests, if you agree. I don't think it could cause a downtime, because it doesn't change data in the table.