Opened 14 years ago

Closed 10 years ago

Last modified 10 years ago

#460 enhancement closed fixed (fixed)

[PATCH] example cred checker for pypgsql/adbapi.

Reported by: matiu Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Glyph, Dave Peticolas, itamarst, matiu, therve Branch:
Author:

Description


Attachments (3)

dbcred.py (6.3 KB) - added by matiu 14 years ago.
example_240.diff (10.5 KB) - added by therve 11 years ago.
example_240_2.diff (6.5 KB) - added by therve 11 years ago.

Download all attachments as: .zip

Change History (25)

Changed 14 years ago by matiu

Attachment: dbcred.py added

comment:1 Changed 14 years ago by matiu

I needed dbcred so I wrote the attached file. If someone could please put it in
twisted that it be cool. I've tested it with the default settings. I'm using it
in my production code. Probably needs more testing though.

Thanks :)

comment:2 Changed 14 years ago by matiu

The contents of the file are:
 A database checker object that implements IChecker

comment:3 Changed 14 years ago by itamarst

Shouldn't be tied to pyPgSQL I don't think, and ought to include sample SQL
schema... I'm not sure this should be in Twisted proper, really. Might make a
good example though, and certainly posting this to the list would be useful to
other people.

comment:4 Changed 14 years ago by Dave Peticolas

This would be a good example. As part of putting it in the examples,
I think we can remove the deprecated code in enterprise.

Changed 11 years ago by therve

Attachment: example_240.diff added

comment:5 Changed 11 years ago by therve

Component: core
Keywords: review added; core removed
Owner: Dave Peticolas deleted
Priority: normalhighest

Let's save dave :).

I've updated the patch to work with current Twisted, and a example to run it (not really clean :/).

comment:6 Changed 11 years ago by Dave Peticolas

Thanks :) I swear to take a look at it this weekend.

comment:7 Changed 11 years ago by Jean-Paul Calderone

Something about this patch bothers me. I guess it is the large amount of fairly non-trivial code with no test coverage.

comment:8 Changed 11 years ago by hypatia

Cc: hypatia removed

comment:9 in reply to:  7 Changed 11 years ago by therve

Cc: therve added

Replying to exarkun:

Something about this patch bothers me. I guess it is the large amount of fairly non-trivial code with no test coverage.

I guess you're talking about the ssh server initialization ? The example itself is pretty simple, but the main function is really dirty. I wanted to add a ssh server because I wanted a easily testable server, but I guess something with pb will be easier.

comment:10 Changed 11 years ago by Dave Peticolas

Thanks for submitting this, therve, you put me to shame :)

Regarding exarkun's, comment, this does look like it would become the largest example in the docs directory, at least in terms of line count. Though some of the length is due to documentation that a lot of the existing examples don't have.

I do think it could be significantly shortened, though. Using PB might be simpler than conch. Also, a lot of the options, like 'caseSensitiveUsernames', don't really contribute to understanding -- anyone could add those once they understood the essentials.

What do you think?

comment:11 Changed 11 years ago by therve

In fact I'd just tak the sql query as parameter, that would prevent all these options. Also I could switch to PB to reduce the main.

comment:12 in reply to:  11 Changed 11 years ago by Dave Peticolas

Replying to therve:

In fact I'd just tak the sql query as parameter, that would prevent all these options. Also I could switch to PB to reduce the main.

Yep, that sounds good.

comment:13 Changed 11 years ago by therve

Keywords: review removed
Owner: set to therve

Changed 11 years ago by therve

Attachment: example_240_2.diff added

comment:14 Changed 11 years ago by therve

Keywords: review added
Owner: therve deleted

OK the main section is now much simpler, and with using pbecho I'm now compatible with pbechoclient, which is good.

comment:15 Changed 11 years ago by Dave Peticolas

This looks good to me, though I think 'caseSensitivePasswords' could probably be left out as well. OTOH, showing a little variation in the auth code might be helpful. Anyone object to putting in this example as is?

comment:16 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

A few minor points:

  1. The file should have a copyright statement.
  2. There are some simple suites in the code, these should be expanded.
  3. The docstring for init talks about PyPgSQL, but the code actually uses PySQLite2. Having two different query strings and only using one makes the example more complex than it needs to be. Likewise with the customCheckFunc, which is not passed in by the driver code. The example should probably focus on one thing and do really well at conveying the details relevant to that thing. Hashed password checking, case insensitivity, and cross-database compatibility could probably each use an independent example in addition to this one.
  4. The module docstring could probably do with some prose giving the overall point of the example.
  5. There's a local variable db_deferred which should be dbDeferred to conform to the naming convention.

Of these, 3 probably isn't a blocker, and the rest are trivial. This is certainly an improvement over the existing documentation situation, but I think it could be even better if we really focus on the concepts we're trying to communicate.

comment:17 Changed 10 years ago by therve

Keywords: review added
Owner: changed from therve to Jean-Paul Calderone

I did the modifications of 1,2,4,5 and some of point 3. I'm reluctant to remove code from the example, as it's a pretty good full-featured example, and I don't think it makes it so complex. We can also provide different main methods to run different examples ?

But I'm not completely insensible to your argument, so if I didn't convince you I'll simplify it :).

comment:18 Changed 10 years ago by therve

I forget, the file is now in branch db-cred-460.

comment:19 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve

I'm not unconvinced, but I'm not completely convinced either. Most of my reservations are based on problems which every other example we have shares though, so I'm will to let this go until we have a general solution we can apply to everything. The problem is just that this is code with no test coverage, so it's probably broken. ;)

Thanks for doing the other cleanups. Just one more thing: I'm not sure we should suggest using the code as-is in the module docstring. It's an example, it's good for helping users get an idea of how this kind of thing works. If they also decide they can copy it into their project and use it, that's their problem. ;) I'd rather not suggest we do this, though (until we have unit tests we can suggest they copy as well, at least).

comment:20 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(In [19793]) Merge db-cred-460

Author: matiu, therve Reviewer: exarkun, davep Fixes #460

Add an example cred checker using adbapi.

comment:21 Changed 10 years ago by therve

Until we find a solution (I'm not sure about unittests... maybe running the examples directly?), we could add a step to the review process 'Try to run the examples that may be changed by the branch'. It's not that great, but it's already a step toward better examples.

comment:22 Changed 7 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.