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

NO BUG - DEMO ONLY - external models - death of dataservice #2687

Closed
wants to merge 1 commit into from
Closed

NO BUG - DEMO ONLY - external models - death of dataservice #2687

wants to merge 1 commit into from

Conversation

twobraids
Copy link
Contributor

the idea has been to move the middleware services into django. the dataservices idea does that, but strives to remain backward compatible by allowing there to exist a dataservices app. I suggest throwing that idea out entirely. We don't need a standalone dataservices app. Let's have our security and authorizations in one location: Django.

We take each of the middleware/dataservice services and write them as a class completely ignorant of any web interface - just pure business logic living an appropriate place in the socorro tree. Then use the dataservices "magic model maker" to import these classes as Django models. The Django views can then use these models just as they would normal Django models.

This gives us several advantages.

  1. it eliminates all the handwaving and extreme measures that we've done to wrap web services with web services. Eliminates all mulitple layers of whitelisting and authentication allowing that to be consolidated into the exposed Django interface
  2. it allows configman to step in and swap model implementations as our needs change in the same smooth manner that the crashstorage classes get to change.
  3. it makes the algorithms that make up the models available to other systems within socorro. I can think of several places where I could use them in processor rules. Having them free of an HTTP interface makes them far more flexible.

WHERE bug_associations.bug_id = bugs.id
AND signature IN %s
)
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you keep these in the __init__ and not as class attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no particular reason from me. Is there a advantage to using class attributes instead?

I larger question would be, why is this one class rather than two? If there is going to be a family of these service implementations, then the common things ought to get pushed to a base class. Which then leads to the idea that this class, with two distinct methods that are only categorically related, ought to be split into two classes.

Service implementation classes such as this one define APIs. Collecting all the API methods into one class would seem to be a laudable goal. The argues the point of not dividing this class into two independent ones.

@adngdb
Copy link
Contributor

adngdb commented Mar 24, 2015

So the way I understand this is quite simple: we move back to the original goal of the external module (some business logic related to an external resource, organized in classes, with no knowledge of the API), and we replace what used to be the middleware with a collection of dynamically generated Django models. I am all in favor of it!

As you already mentioned, this is the opportunity to discuss some design decisions.

Having one class per service makes it easier to generate documentation about services, but it couples the implementations to the public interface (since there's no real reason to keep to one method per class) (for example, all Super Search Fields methods are in one class, socorro.external.es.super_search_fields, because those are all related, and I find it hard to justify separating those from a design point of view). It's possible that enable automatic documentation generation is entirely worth sacrificing some design coherence.

The current external module is already not related to HTTP (or so it should be). We use specific exceptions for common errors (defined in external/init.py). I have tried to have names that are as much as possible away from HTTP vocabulary (create instead of post, update instead of put).

The question that bothers me is: how do we associate API URLs to a class/method combination in this model? The way it works now, all models define some HTTP verbs (the class has one or several of get, post, put and delete methods), and we simply map a GET to the association method, the class name being in the URL. If we want to decouple the implementation from the HTTP API, we can't do that. That's why when I designed the middleware implementations, I came up with this simple schema: VERB /resource/{aggregation/} -> Resource.verb{_aggregation}. This is explained here: http://socorro.readthedocs.org/en/latest/development/addaservice.html#naming-conventions

I don't know what that's worth, but at least that's the best I could think of when I was working on this problem, so at least I had to mention it. :)

@peterbe
Copy link
Contributor

peterbe commented Apr 1, 2015

We'll continue this effort in the April workweek.

@peterbe peterbe closed this Apr 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants