{"action":"created","issue":{"url":"https://api.github.com/repos/django/django/issues/6254","repository_url":"https://api.github.com/repos/django/django","labels_url":"https://api.github.com/repos/django/django/issues/6254/labels{/name}","comments_url":"https://api.github.com/repos/django/django/issues/6254/comments","events_url":"https://api.github.com/repos/django/django/issues/6254/events","html_url":"https://github.com/django/django/pull/6254","id":139273019,"number":6254,"title":"Fixed #26292 Refactor MigrationLoader.build_graph()","user":{"login":"jarekwg","id":5036488,"avatar_url":"https://avatars.githubusercontent.com/u/5036488?v=3","gravatar_id":"","url":"https://api.github.com/users/jarekwg","html_url":"https://github.com/jarekwg","followers_url":"https://api.github.com/users/jarekwg/followers","following_url":"https://api.github.com/users/jarekwg/following{/other_user}","gists_url":"https://api.github.com/users/jarekwg/gists{/gist_id}","starred_url":"https://api.github.com/users/jarekwg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jarekwg/subscriptions","organizations_url":"https://api.github.com/users/jarekwg/orgs","repos_url":"https://api.github.com/users/jarekwg/repos","events_url":"https://api.github.com/users/jarekwg/events{/privacy}","received_events_url":"https://api.github.com/users/jarekwg/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":12,"created_at":"2016-03-08T13:03:27Z","updated_at":"2016-03-25T14:46:24Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/django/django/pulls/6254","html_url":"https://github.com/django/django/pull/6254","diff_url":"https://github.com/django/django/pull/6254.diff","patch_url":"https://github.com/django/django/pull/6254.patch"},"body":"[Ticket #26292](https://code.djangoproject.com/ticket/26292)\r\n\r\nThis PR also solves [Ticket #25945](https://code.djangoproject.com/ticket/25945), including the tests from PR #5939 along with @timgraham's recommended changes to them.\r\n\r\n@MarkusH, Adding only normal nodes at the first step proved problematic, as replacing migrations can still reference other replacing migrations, as well as there being possible hiccups that could arise with `__first__`. So the solution I found was to add _all_ nodes to the graph (both normal and replacing migrations), including all their listed dependencies, and _then_ iterate over the replacing migrations and either delete them, or the migrations they replace from the graph, remapping the dependencies as this happens. This ordering seems to work well.\r\n\r\nBelow are some things I would like feedback on:\r\n1. Since we're now creating dummy nodes instead of immediately raising a `NodeNotFoundError`, for `validate_consistency` to give a meaningful error upon finding dangling dummy nodes, it needs to know about the migration that triggered the creation of the given dummy node. To achieve this, I create the exception when I create the dummy node, and then `validate_consistency` raises that exception if that dummy node still exists at the end. However I'm not hugely happy with the implementation (have a look at the code..).\r\n2. Speed? We're creating and deleting more objects now, as opposed to the previous method of resolving everything in a bunch of dicts prior to building the graph. Might this be visibly less performant, or is this not significant enough to be a worry?\r\n3. I haven't seen any tests for `__first__`, `__latest__` or `run_before` being used in conjunction with squashed migrations. Should we add some in this PR?\r\n4. Is there a point to storing both `graph.node_map` and `graph.nodes`? Could the Migration object not stem off the Node object, rather than sitting in a separate dict? Specifically, I don’t like the doubling up between `graph.nodes[x].dependencies` and `graph.node_map[x].parents`. The current implementation updates `Migration.dependencies` to ensure consistency with the graph. My implementation currently doesn't touch `Migration.dependencies` (Hence one of the tests not passing). I’d be inclined to just delete the `Migration.dependencies` (same with `.replaces` and `.run_before`) lists after the graph has been built from them. Or we should just not touch them after they’ve been loaded in (though deleting would prevent confusion and accidental use). Currently the `showmigrations` mcommand uses `Migration.dependencies` rather than `Node.parents`. Should it really be displaying this? Seems a bit weird to me..\r\n\r\nAlso, `NodeNotFoundError`’s `__repr__` had a bug: have to call `repr(node)` before passing it to `%r` formatter. Fixed.\r\n\r\nThis is my first refactor of Django code; please pick it to bits with utmost scrutiny. :)"},"comment":{"url":"https://api.github.com/repos/django/django/issues/comments/201319551","html_url":"https://github.com/django/django/pull/6254#issuecomment-201319551","issue_url":"https://api.github.com/repos/django/django/issues/6254","id":201319551,"user":{"login":"jarekwg","id":5036488,"avatar_url":"https://avatars.githubusercontent.com/u/5036488?v=3","gravatar_id":"","url":"https://api.github.com/users/jarekwg","html_url":"https://github.com/jarekwg","followers_url":"https://api.github.com/users/jarekwg/followers","following_url":"https://api.github.com/users/jarekwg/following{/other_user}","gists_url":"https://api.github.com/users/jarekwg/gists{/gist_id}","starred_url":"https://api.github.com/users/jarekwg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jarekwg/subscriptions","organizations_url":"https://api.github.com/users/jarekwg/orgs","repos_url":"https://api.github.com/users/jarekwg/repos","events_url":"https://api.github.com/users/jarekwg/events{/privacy}","received_events_url":"https://api.github.com/users/jarekwg/received_events","type":"User","site_admin":false},"created_at":"2016-03-25T14:46:24Z","updated_at":"2016-03-25T14:46:24Z","body":"Okay, I think I've just about wrapped up everything I wanted to do here.\r\n\r\nOne final thing I'm not sure about is whether it's possible for a migration to be listed in more than one `replaces` list (ie. replaced by more than one replacing migration). I've made the assumption that it isn't allowed (don't know how it would even make sense if it was), but just thought I'd confirm this, as I'm not sure whether the existing code makes this simplifying assumption.\r\n\r\nPlease also advise me after your next review as to what extra documentation needs to be provided before a PR like this can be merged in. I'm guessing it's just a changelog entry? Or is there other stuff too?"}}