Skip to content

Add support for reverse dependencies. #69

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

Merged
merged 3 commits into from
Nov 24, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/models/crate.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ export default DS.Model.extend({
owners: DS.hasMany('users', {async: true}),
version_downloads: DS.hasMany('version-download', {async: true}),
keywords: DS.hasMany('keywords', {async: true}),
reverse_dependencies: DS.hasMany('reverse-dependency', {async: true}),
Copy link
Member

Choose a reason for hiding this comment

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

Here should should be able to reuse the same model like:

reverse_dependencies: DS.hasMany('dependencies', {async: true}),

Copy link
Member Author

Choose a reason for hiding this comment

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

I basically just poked at things until they worked, I started with that, but it didn't work, so I cargo-culted from the other files here. I don't yet know what I'm doing when it comes to Ember.

If I change to reverse_dependencies: DS.hasMany('dependency', {async: true}) & delete the reverse dependencies model I get:

No model was found for 'reverseDependency'

  EmberError@http://localhost:4200/assets/vendor.js:26705:17
Store<.modelFor@http://localhost:4200/assets/vendor.js:72854:1
__exports__.default<.extractArray@http://localhost:4200/assets/vendor.js:66153:22
apply@http://localhost:4200/assets/vendor.js:31551:1
superWrapper@http://localhost:4200/assets/vendor.js:31125:15
__exports__.default<.extractFindHasMany@http://localhost:4200/assets/vendor.js:65470:16
__exports__.default<.extract@http://localhost:4200/assets/vendor.js:65405:16
_findHasMany/<@http://localhost:4200/assets/vendor.js:73422:23
tryCatch@http://localhost:4200/assets/vendor.js:58985:16
invokeCallback@http://localhost:4200/assets/vendor.js:58997:17
publish@http://localhost:4200/assets/vendor.js:58968:11
@http://localhost:4200/assets/vendor.js:42236:9
DeferredActionQueues.prototype.invoke@http://localhost:4200/assets/vendor.js:13801:11
DeferredActionQueues.prototype.flush@http://localhost:4200/assets/vendor.js:13851:15
Backburner.prototype.end@http://localhost:4200/assets/vendor.js:13314:11
Backburner.prototype.run@http://localhost:4200/assets/vendor.js:13369:15
apply@http://localhost:4200/assets/vendor.js:31549:1
run@http://localhost:4200/assets/vendor.js:30166:14
__exports__.default<.ajax/</hash.success@http://localhost:4200/assets/vendor.js:63875:15
jQuery.Callbacks/fire@http://localhost:4200/assets/vendor.js:3230:10
jQuery.Callbacks/self.fireWith@http://localhost:4200/assets/vendor.js:3342:7
done@http://localhost:4200/assets/vendor.js:9386:5
.send/callback@http://localhost:4200/assets/vendor.js:9796:8

If I leave the reverse-dependency.js model and just change this line I get:

Assertion Failed: The response from a findHasMany must be an Array, not undefined

  EmberError@http://localhost:4200/assets/vendor.js:26705:17
Ember.assert@http://localhost:4200/assets/vendor.js:16889:15
_findHasMany/<@http://localhost:4200/assets/vendor.js:73424:1
tryCatch@http://localhost:4200/assets/vendor.js:58985:16
invokeCallback@http://localhost:4200/assets/vendor.js:58997:17
publish@http://localhost:4200/assets/vendor.js:58968:11
@http://localhost:4200/assets/vendor.js:42236:9
DeferredActionQueues.prototype.invoke@http://localhost:4200/assets/vendor.js:13801:11
DeferredActionQueues.prototype.flush@http://localhost:4200/assets/vendor.js:13851:15
Backburner.prototype.end@http://localhost:4200/assets/vendor.js:13314:11
Backburner.prototype.run@http://localhost:4200/assets/vendor.js:13369:15
apply@http://localhost:4200/assets/vendor.js:31549:1
run@http://localhost:4200/assets/vendor.js:30166:14
__exports__.default<.ajax/</hash.success@http://localhost:4200/assets/vendor.js:63875:15
jQuery.Callbacks/fire@http://localhost:4200/assets/vendor.js:3230:10
jQuery.Callbacks/self.fireWith@http://localhost:4200/assets/vendor.js:3342:7
done@http://localhost:4200/assets/vendor.js:9386:5
.send/callback@http://localhost:4200/assets/vendor.js:9796:8

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! (this may be out of my ember-knowledge territory)

The line above:

owners: DS.hasMany('users', {async: true}),

makes me think that this should work, but I'm not entirely sure why, I'll try to investigate.

});
3 changes: 3 additions & 0 deletions app/models/reverse-dependency.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import Dependency from 'cargo/models/dependency';

export default Dependency;
Copy link
Member

Choose a reason for hiding this comment

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

This may not be quite what we want because this means that the caches aren't shared, I'll comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does sharing the caches make sense, dependencies and reverse dependencies refer to different things?

Copy link
Member

Choose a reason for hiding this comment

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

More caching in the sense that if we've already fetched the information for dependency 5 there's no need to re-fetch the information just because it's a reverse dependency (it'll be the same information).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the information is slightly different: if a depends on b, then the associated Dependency.crate_id for a will hold b, but the associated ReverseDependency.crate_id for b will hold a.

(I'm almost certainly misunderstanding what you mean.)

1 change: 1 addition & 0 deletions app/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Router.map(function() {
this.resource('crate', { path: '/crates/*crate_id' }, function() {
this.route('download');
this.route('versions');
this.route('reverse_dependencies');
});
this.route('me', function() {
this.route('crates');
Expand Down
20 changes: 20 additions & 0 deletions app/routes/crate/reverse-dependencies.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import Ember from 'ember';
import Crate from 'cargo/models/crate';

export default Ember.Route.extend({
afterModel: function(data) {
console.log("afterModel");
if (data instanceof Crate) {
return data.get('reverse_dependencies');
} else {
return data.crate.get('reverse_dependencies');
}
},

setupController: function(controller, data) {
if (data instanceof Crate) {
data = {crate: data, reverse_dependencies: null};
}
this._super(controller, data.crate);
},
});
24 changes: 24 additions & 0 deletions app/templates/crate/reverse-dependencies.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<div class='all-versions-back'>
{{#link-to 'crate' this}}&#11013; Back to Main Page{{/link-to}}
</div>

<div class='info'>
<span class='small'>
All <span class='num'>{{ reverse_dependencies.length }}</span>
reverse dependencies of <span class='num'>{{ name }}</span>
</span>
</div>

<div id='crate-all-reverse-dependencies' class='white-rows'>
{{#each model.reverse_dependencies}}
<div class='row'>
<div>
{{#link-to 'crate' crate_id}}{{crate_id}}{{/link-to}} requires {{req}}
{{#link-to 'crate' this}}{{ num }}{{/link-to}}
</div>
{{#link-to 'crate' this class='arrow'}}
<img src="/assets/right-arrow-all-versions.png"/>
{{/link-to}}
</div>
{{/each}}
</div>
34 changes: 34 additions & 0 deletions src/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use url::{mod, Url};
use {Model, User, Keyword, Version};
use app::{App, RequestApp};
use db::{Connection, RequestTransaction};
use dependency::{Dependency, EncodableDependency};
use download::{VersionDownload, EncodableVersionDownload};
use git;
use keyword::EncodableKeyword;
Expand Down Expand Up @@ -68,6 +69,7 @@ pub struct CrateLinks {
pub version_downloads: String,
pub versions: Option<String>,
pub owners: Option<String>,
pub reverse_dependencies: String,
}

impl Crate {
Expand Down Expand Up @@ -253,6 +255,7 @@ impl Crate {
version_downloads: format!("/api/v1/crates/{}/downloads", name),
versions: versions_link,
owners: Some(format!("/api/v1/crates/{}/owners", name)),
reverse_dependencies: format!("/api/v1/crates/{}/reverse_dependencies", name)
},
}
}
Expand Down Expand Up @@ -333,6 +336,22 @@ impl Crate {
let rows = try!(stmt.query(&[&self.id]));
Ok(rows.map(|r| Model::from_row(&r)).collect())
}

/// Returns (dependency, dependent crate name)
pub fn reverse_dependencies(&self, conn: &Connection) -> CargoResult<Vec<(Dependency, String)>> {
let stmt = try!(conn.prepare("SELECT dependencies.*,
crates.name AS crate_name
FROM dependencies
INNER JOIN versions
ON versions.id = dependencies.version_id
INNER JOIN crates
ON crates.id = versions.crate_id
WHERE dependencies.crate_id = $1
AND versions.num = crates.max_version"));
Ok(try!(stmt.query(&[&self.id])).map(|r| {
(Model::from_row(&r), r.get("crate_name"))
}).collect())
}
}

impl Model for Crate {
Expand Down Expand Up @@ -860,3 +879,18 @@ fn modify_owners(req: &mut Request, add: bool) -> CargoResult<Response> {
struct R { ok: bool }
Ok(req.json(&R{ ok: true }))
}

pub fn reverse_dependencies(req: &mut Request) -> CargoResult<Response> {
let name = &req.params()["crate_id"];
let conn = try!(req.tx());
let krate = try!(Crate::find_by_name(conn, name.as_slice()));
let tx = try!(req.tx());
let rev_deps = try!(krate.reverse_dependencies(tx));
let rev_deps = rev_deps.into_iter().map(|(dep, crate_name)| {
dep.encodable(crate_name.as_slice())
}).collect();

#[deriving(Encodable)]
struct R { reverse_dependencies: Vec<EncodableDependency> }
Ok(req.json(&R{ reverse_dependencies: rev_deps }))
}
1 change: 1 addition & 0 deletions src/lib.rs
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ pub fn middleware(app: Arc<App>) -> MiddlewareBuilder {
api_router.delete("/crates/:crate_id/owners", C(krate::remove_owners));
api_router.delete("/crates/:crate_id/:version/yank", C(version::yank));
api_router.put("/crates/:crate_id/:version/unyank", C(version::unyank));
api_router.get("/crates/:crate_id/reverse_dependencies", C(krate::reverse_dependencies));
api_router.get("/versions", C(version::index));
api_router.get("/versions/:version_id", C(version::show));
api_router.get("/keywords", C(keyword::index));
Expand Down
15 changes: 10 additions & 5 deletions src/tests/all.rs
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use conduit_test::MockRequest;
use cargo_registry::app::App;
use cargo_registry::db::{mod, RequestTransaction};
use cargo_registry::{User, Crate, Version, Keyword};
use cargo_registry::util::CargoResult;

macro_rules! t( ($e:expr) => (
match $e {
Expand Down Expand Up @@ -213,8 +214,14 @@ fn mock_user(req: &mut Request, u: User) -> User {
}

fn mock_crate(req: &mut Request, krate: Crate) -> Crate {
let (c, v) = mock_crate_vers(req, krate, &semver::Version::parse("1.0.0").unwrap());
v.unwrap();
c
}
fn mock_crate_vers(req: &mut Request, krate: Crate, v: &semver::Version)
-> (Crate, CargoResult<Version>) {
let user = req.extensions().find::<User>().unwrap();
let krate = Crate::find_or_insert(req.tx().unwrap(), krate.name.as_slice(),
let mut krate = Crate::find_or_insert(req.tx().unwrap(), krate.name.as_slice(),
user.id, &krate.description,
&krate.homepage,
&krate.documentation,
Expand All @@ -224,10 +231,8 @@ fn mock_crate(req: &mut Request, krate: Crate) -> Crate {
&krate.license).unwrap();
Keyword::update_crate(req.tx().unwrap(), &krate,
krate.keywords.as_slice()).unwrap();
Version::insert(req.tx().unwrap(), krate.id,
&semver::Version::parse("1.0.0").unwrap(),
&HashMap::new(), &[]).unwrap();
return krate;
let v = krate.add_version(req.tx().unwrap(), v, &HashMap::new(), &[]);
(krate, v)
}

fn mock_keyword(req: &mut Request, name: &str) -> Keyword {
Expand Down
33 changes: 32 additions & 1 deletion src/tests/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ struct CrateResponse { krate: EncodableCrate, versions: Vec<EncodableVersion> }
#[deriving(Decodable)]
struct Deps { dependencies: Vec<EncodableDependency> }
#[deriving(Decodable)]
struct RevDeps { reverse_dependencies: Vec<EncodableDependency> }
#[deriving(Decodable)]
struct Downloads { version_downloads: Vec<EncodableVersionDownload> }

#[test]
Expand Down Expand Up @@ -504,7 +506,7 @@ fn dependencies() {
let c1 = ::krate("foo");
let c2 = ::krate("bar");
middle.add(::middleware::MockUser(user.clone()));
middle.add(::middleware::MockDependency(c1.clone(), c2.clone()));
middle.add(::middleware::MockDependency(c1.clone(), "1.0.0", c2.clone()));
let rel = format!("/api/v1/crates/{}/1.0.0/dependencies", c1.name);
let mut req = MockRequest::new(conduit::Get, rel.as_slice());
let mut response = ok_resp!(middle.call(&mut req));
Expand Down Expand Up @@ -700,3 +702,32 @@ fn bad_keywords() {
::json::<::Bad>(&mut response);
}
}

#[test]
fn reverse_dependencies() {
let (_b, _app, mut middle) = ::app();
let user = ::user("foo");
let c1 = ::krate("foo");
let c2 = ::krate("bar");
middle.add(::middleware::MockUser(user.clone()));
// multiple dependencies of c1 on c2, to detect we handle this
// correctly.
middle.add(::middleware::MockDependency(c1.clone(), "1.0.0", c2.clone()));
middle.add(::middleware::MockDependency(c1.clone(), "1.1.0", c2.clone()));

let rel = format!("/api/v1/crates/{}/reverse_dependencies", c2.name);
let mut req = MockRequest::new(conduit::Get, rel.as_slice());
let mut response = ok_resp!(middle.call(&mut req));
let deps = ::json::<RevDeps>(&mut response);
assert_eq!(deps.reverse_dependencies.len(), 1);
assert_eq!(deps.reverse_dependencies[0].crate_id.as_slice(), &*c1.name);
drop(req);

// c1 has no dependent crates.
let rel = format!("/api/v1/crates/{}/reverse_dependencies", c1.name);
let mut req = MockRequest::new(conduit::Get, rel.as_slice());
let mut response = ok_resp!(middle.call(&mut req));
let deps = ::json::<RevDeps>(&mut response);
assert_eq!(deps.reverse_dependencies.len(), 0);
drop(req);
}
16 changes: 10 additions & 6 deletions src/tests/middleware.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,19 @@ impl Middleware for MockCrate {
}
}

pub struct MockDependency(pub Crate, pub Crate);
pub struct MockDependency(pub Crate, pub &'static str, pub Crate);

impl Middleware for MockDependency {
fn before(&self, req: &mut Request) -> Result<(), Box<Show + 'static>> {
let MockDependency(ref a, ref b) = *self;
let crate_a = ::mock_crate(req, a.clone());
let crate_b = ::mock_crate(req, b.clone());
let va = crate_a.versions(req.tx().unwrap()).unwrap()[0].id;
Dependency::insert(req.tx().unwrap(), va, crate_b.id,
let MockDependency(ref a, version, ref b) = *self;
let vers = semver::Version::parse(version).unwrap();
let (_crate_a, va) = ::mock_crate_vers(req, a.clone(), &vers);

// don't panic on duplicate uploads
let (crate_b, _) = ::mock_crate_vers(req, b.clone(),
&semver::Version::parse("1.0.0").unwrap());

Dependency::insert(req.tx().unwrap(), va.unwrap().id, crate_b.id,
&semver::VersionReq::parse(">= 0").unwrap(),
Kind::Normal,
false, true, &[], &None).unwrap();
Expand Down