<p dir="auto">Race conditions around bad transaction isolation in FAB results in dups in ab_permission_view.</p>
<p dir="auto">Starting multiple threads at once when kicking off gunicorn leads to the "insert if not exist" logic to misfire. Root cause is bad transaction isolation in FAB.</p>
<div class="highlight highlight-source-sql notranslate position-relative overflow-auto" dir="auto" data-snippet-clipboard-copy-content=" select c.name as perm_name, b.name as view_menu_name, count(1)
 from ab_permission_view a
 join ab_view_menu b on b.id = a.view_menu_id
 join ab_permission c on c.id = a.permission_id 
 group by c.name, b.name
 having count(1) &gt;1"><pre class="notranslate"> <span class="pl-k">select</span> <span class="pl-c1">c</span>.<span class="pl-c1">name</span> <span class="pl-k">as</span> perm_name, <span class="pl-c1">b</span>.<span class="pl-c1">name</span> <span class="pl-k">as</span> view_menu_name, <span class="pl-c1">count</span>(<span class="pl-c1">1</span>)
 <span class="pl-k">from</span> ab_permission_view a
 <span class="pl-k">join</span> ab_view_menu b <span class="pl-k">on</span> <span class="pl-c1">b</span>.<span class="pl-c1">id</span> <span class="pl-k">=</span> <span class="pl-c1">a</span>.<span class="pl-c1">view_menu_id</span>
 <span class="pl-k">join</span> ab_permission c <span class="pl-k">on</span> <span class="pl-c1">c</span>.<span class="pl-c1">id</span> <span class="pl-k">=</span> <span class="pl-c1">a</span>.<span class="pl-c1">permission_id</span> 
 <span class="pl-k">group by</span> <span class="pl-c1">c</span>.<span class="pl-c1">name</span>, <span class="pl-c1">b</span>.<span class="pl-c1">name</span>
 <span class="pl-k">having</span> <span class="pl-c1">count</span>(<span class="pl-c1">1</span>) <span class="pl-k">&gt;</span><span class="pl-c1">1</span></pre></div>
<p dir="auto">Since we know this problem exist, we should make <code class="notranslate">superset init</code> fix this issue and eventually get to prevent it in FAB.</p>
<p dir="auto">Identical <code class="notranslate">permission_view</code> objects essentially need to get merged into roles.</p>
<h2 dir="auto">[SIP] Proposal for Improving Superset’s Python Code Organization</h2>
<h3 dir="auto">Motivation</h3>
<p dir="auto">As I was in the weeds fixing a bunch of Pylint failures, Max and I started going back and forth on this PR: <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="533661521" data-permission-text="Title is private" data-url="https://github.com/apache/superset/issues/8777" data-hovercard-type="pull_request" data-hovercard-url="/apache/superset/pull/8777/hovercard" href="https://github.com/apache/superset/pull/8777">#8777</a>, which we ultimately closed. The root cause of that was a lack of shared understanding on the best code structure for Superset going forward. Talking with others at Preset, I realized that the issue was larger than just a new contributor not understanding project practices. Without a shared understanding, we are lacking a cohesive approach towards refactoring Superset - we need a technical North Star for project structure.</p>
<p dir="auto">Preset’s developers met and identified a number of pain points in the existing code base. Here they are, with a bit of color to make the meaning clear:</p>
<ul dir="auto">
<li>Lack of separation of concerns
<ul dir="auto">
<li>There are a number of modules that have grown organically over time to truly massive size. Often these modules contain utility functions, classes of multiple types, and a broad array of other functionality. These modules are very hard to work with.</li>
<li>Business logic is frequently intermixed with display logic, making code reuse more difficult.</li>
<li>The model layer contains a large amount of business logic which has nothing to do with data storage.</li>
</ul>
</li>
<li>Dependency Confusion
<ul dir="auto">
<li>Circular dependency issues crop up regularly with the existing module design</li>
<li>Many modules have implicit dependencies on other modules which requires a specific load order</li>
</ul>
</li>
<li>Lack of Composability
<ul dir="auto">
<li>There’s a substantial amount of app-specific code in shared modules</li>
<li>Views are currently loaded in all contexts, which makes light-weight command line usage (for example) impossible</li>
</ul>
</li>
<li>Deep Class Inheritance
<ul dir="auto">
<li>In some instances, particularly where there is pre-existing inheritance in Flask-AppBuilder, the inheritance chain can be over 10 classes deep. This can make understanding some classes very challenging.</li>
</ul>
</li>
</ul>
<h3 dir="auto">Proposed Change</h3>
<p dir="auto">In order to address these concerns, we’d like to propose a few guiding principles for Python code structure going forward:</p>
<ul dir="auto">
<li>Prefer small, functionally-homogeneous modules with only classes and pure functions in module scope</li>
<li>Separate business logic from display logic
<ul dir="auto">
<li>Keep the view layer lightweight and delegate to business logic objects</li>
<li>To support this work, we’d like to recommend a Command pattern - more on that below</li>
</ul>
</li>
<li>Prefer thin ORM objects
<ul dir="auto">
<li>To support this work, we’d like to recommend introducing Data Access Objects or DAOs to Superset to hold complicated query logic. More on that below.</li>
</ul>
</li>
<li>Keep <code class="notranslate">__init__.py</code> files empty
<ul dir="auto">
<li>This will help cut down on implicit dependencies</li>
</ul>
</li>
<li>Prefer composition over inheritance
<ul dir="auto">
<li>Collaborator objects can be simpler to understand than a deeply-nested class hierarchy.</li>
</ul>
</li>
<li>Organize modules function and back-end objects rather than products
<ul dir="auto">
<li>Prefer:
<ul dir="auto">
<li>Dashboard</li>
<li>Chart</li>
<li>Query</li>
<li>Database</li>
<li>Models</li>
<li>api</li>
</ul>
</li>
<li>Avoid:
<ul dir="auto">
<li>SQL Lab</li>
<li>Explore</li>
</ul>
</li>
</ul>
</li>
</ul>
<h4 dir="auto">New patterns to introduce:</h4>
<h5 dir="auto">Command:</h5>
<p dir="auto">There are multiple patterns named Command, and the one we reference here is most similar to an Alembic migration. Commands perform a single cohesive business operation, have a very small public API, and either entirely succeed or entirely fail. In practice, this requires that database transaction behavior be controlled at the Command level. When possible, commands should be modeled such that they perform a single database transaction. In the case of failure, they raise an exception or return an error code, and it is the responsibility of the caller to translate that into a correct response for a user.</p>
<p dir="auto">Example command:</p>
<div class="highlight highlight-source-python notranslate position-relative overflow-auto" dir="auto" data-snippet-clipboard-copy-content="# This is obviously not a full finished command. The point
# is to illustrate the structure of a command as
# encapsulating a business operation, and illustrate
# a likely public API.

import DashboardDAO from daos.dashboard

class MissingDashboardError(CommandError):
  pass

class UpdateFailedError(CommandError):
  pass

class InvalidDashboardError(CommandError):
  pass

class UpdateDashboardCommand &lt; Command:
  def __init__(current_user, dashboard_id, dashboard_properties):
    self.actor = current_user
    self.dashboard_id = dashboard_id
    self.dashboard_properties = dashboard_properties
    
  def run():
    self.dashboard = DashboardDAO.find_dashboard(actor, dashboard_id)
    if self.dashboard IS NONE:
      raise MissingDashboardError(&quot;Dashboard could not be found&quot;)
      
    if !self._is_dashboard_valid():
      raise InvalidDashboardError(&quot;Dashboard parameters are invalid.&quot;)
    
    update_result = DashboardDAO.update(dashboard, dashboard_properties)
    
    if update_result != True:
      raise UpdateFailedError(&quot;Dashboard could not be updated.&quot;)
      
  
  def _is_dashboard_valid():
    # Check validity of dashboard data before save"><pre class="notranslate"><span class="pl-c"># This is obviously not a full finished command. The point</span>
<span class="pl-c"># is to illustrate the structure of a command as</span>
<span class="pl-c"># encapsulating a business operation, and illustrate</span>
<span class="pl-c"># a likely public API.</span>

<span class="pl-k">import</span> <span class="pl-v">DashboardDAO</span> <span class="pl-s1">from</span> <span class="pl-s1">daos</span>.<span class="pl-s1">dashboard</span>

<span class="pl-k">class</span> <span class="pl-v">MissingDashboardError</span>(<span class="pl-v">CommandError</span>):
  <span class="pl-k">pass</span>

<span class="pl-k">class</span> <span class="pl-v">UpdateFailedError</span>(<span class="pl-v">CommandError</span>):
  <span class="pl-k">pass</span>

<span class="pl-k">class</span> <span class="pl-v">InvalidDashboardError</span>(<span class="pl-v">CommandError</span>):
  <span class="pl-k">pass</span>

<span class="pl-k">class</span> <span class="pl-v">UpdateDashboardCommand</span> <span class="pl-c1">&lt;</span> <span class="pl-v">Command</span>:
  <span class="pl-k">def</span> <span class="pl-en">__init__</span>(<span class="pl-s1">current_user</span>, <span class="pl-s1">dashboard_id</span>, <span class="pl-s1">dashboard_properties</span>):
    <span class="pl-s1">self</span>.<span class="pl-s1">actor</span> <span class="pl-c1">=</span> <span class="pl-s1">current_user</span>
    <span class="pl-s1">self</span>.<span class="pl-s1">dashboard_id</span> <span class="pl-c1">=</span> <span class="pl-s1">dashboard_id</span>
    <span class="pl-s1">self</span>.<span class="pl-s1">dashboard_properties</span> <span class="pl-c1">=</span> <span class="pl-s1">dashboard_properties</span>
    
  <span class="pl-k">def</span> <span class="pl-en">run</span>():
    <span class="pl-s1">self</span>.<span class="pl-s1">dashboard</span> <span class="pl-c1">=</span> <span class="pl-v">DashboardDAO</span>.<span class="pl-en">find_dashboard</span>(<span class="pl-s1">actor</span>, <span class="pl-s1">dashboard_id</span>)
    <span class="pl-k">if</span> <span class="pl-s1">self</span>.<span class="pl-s1">dashboard</span> <span class="pl-v">IS</span> <span class="pl-v">NONE</span>:
      <span class="pl-k">raise</span> <span class="pl-v">MissingDashboardError</span>(<span class="pl-s">"Dashboard could not be found"</span>)
      
    <span class="pl-k">if</span> !s<span class="pl-s1">elf</span>.<span class="pl-en">_is_dashboard_valid</span>():
      <span class="pl-k">raise</span> <span class="pl-v">InvalidDashboardError</span>(<span class="pl-s">"Dashboard parameters are invalid."</span>)
    
    <span class="pl-s1">update_result</span> <span class="pl-c1">=</span> <span class="pl-v">DashboardDAO</span>.<span class="pl-en">update</span>(<span class="pl-s1">dashboard</span>, <span class="pl-s1">dashboard_properties</span>)
    
    <span class="pl-k">if</span> <span class="pl-s1">update_result</span> <span class="pl-c1">!=</span> <span class="pl-c1">True</span>:
      <span class="pl-k">raise</span> <span class="pl-v">UpdateFailedError</span>(<span class="pl-s">"Dashboard could not be updated."</span>)
      
  
  <span class="pl-k">def</span> <span class="pl-en">_is_dashboard_valid</span>():
    <span class="pl-c"># Check validity of dashboard data before save</span></pre></div>
<h5 dir="auto">DAO (Data Access Object):</h5>
<p dir="auto">A DAO in the context of Superset would be an object that would manage the lifecycle of SQLAlchemy models. Custom queries would be implemented in DAOs and then called from Command objects. In comparison to Command objects, DAOs have relatively broad public interfaces. DAOs should not depend on other DAOs to avoid circular dependencies. If results are needed cross-DAO, that should be orchestrated in the Command. Here’s a sample simplified DAO for illustrative purposes:</p>
<div class="highlight highlight-source-python notranslate position-relative overflow-auto" dir="auto" data-snippet-clipboard-copy-content="# This is not a fully finished DAO. The intention
# is to illustrate the public interface of a 
# Data Access Object, and how it might interface
# with a SQLAlchemy model.

from superset import db
import DataAccessObject from daos.base
import Dashboard from models.dashboard

class DashboardDAO(DataAccessObject):
  
  @staticmethod
  def find_for_update(actor, dashboard_id):
    dashboard = db.session.query(Dashboard).filter(id=dashboard_id).one_or_none()
    if dashboard
      if actor in dashboard.owners:
        return dashboard
      else
        return None
    else
      return None
  
  @staticmethod
  def update(dashboard, dashboard_params):
    for key, value in dashboard_params.iteritems():
      setattr(dashboard, key, value)
    try:
      db.session.merge(dashboard)
      db.session.commit()
    except SQLAlchemyError as e:
      logger.error('Failed to update dashboard: ?', e.message)
      db.session.rollback()
      return False
      
    return True"><pre class="notranslate"><span class="pl-c"># This is not a fully finished DAO. The intention</span>
<span class="pl-c"># is to illustrate the public interface of a </span>
<span class="pl-c"># Data Access Object, and how it might interface</span>
<span class="pl-c"># with a SQLAlchemy model.</span>

<span class="pl-k">from</span> <span class="pl-s1">superset</span> <span class="pl-k">import</span> <span class="pl-s1">db</span>
<span class="pl-k">import</span> <span class="pl-v">DataAccessObject</span> <span class="pl-k">from</span> <span class="pl-s1">daos</span>.<span class="pl-s1">base</span>
<span class="pl-k">import</span> <span class="pl-v">Dashboard</span> <span class="pl-s1">from</span> <span class="pl-s1">models</span>.<span class="pl-s1">dashboard</span>

<span class="pl-k">class</span> <span class="pl-v">DashboardDAO</span>(<span class="pl-v">DataAccessObject</span>):
  
  <span class="pl-en">@<span class="pl-s1">staticmethod</span></span>
  <span class="pl-k">def</span> <span class="pl-en">find_for_update</span>(<span class="pl-s1">actor</span>, <span class="pl-s1">dashboard_id</span>):
    <span class="pl-s1">dashboard</span> <span class="pl-c1">=</span> <span class="pl-s1">db</span>.<span class="pl-s1">session</span>.<span class="pl-en">query</span>(<span class="pl-v">Dashboard</span>).<span class="pl-en">filter</span>(<span class="pl-s1">id</span><span class="pl-c1">=</span><span class="pl-s1">dashboard_id</span>).<span class="pl-en">one_or_none</span>()
    <span class="pl-k">if</span> <span class="pl-s1">dashboard</span>
      <span class="pl-k">if</span> <span class="pl-s1">actor</span> <span class="pl-c1">in</span> <span class="pl-s1">dashboard</span>.<span class="pl-s1">owners</span>:
        <span class="pl-k">return</span> <span class="pl-s1">dashboard</span>
      <span class="pl-k">else</span>
        <span class="pl-k">return</span> <span class="pl-c1">None</span>
    <span class="pl-s1">else</span>
      <span class="pl-k">return</span> <span class="pl-c1">None</span>
  
  <span class="pl-en">@<span class="pl-s1">staticmethod</span></span>
  <span class="pl-k">def</span> <span class="pl-en">update</span>(<span class="pl-s1">dashboard</span>, <span class="pl-s1">dashboard_params</span>):
    <span class="pl-k">for</span> <span class="pl-s1">key</span>, <span class="pl-s1">value</span> <span class="pl-c1">in</span> <span class="pl-s1">dashboard_params</span>.<span class="pl-en">iteritems</span>():
      <span class="pl-en">setattr</span>(<span class="pl-s1">dashboard</span>, <span class="pl-s1">key</span>, <span class="pl-s1">value</span>)
    <span class="pl-k">try</span>:
      <span class="pl-s1">db</span>.<span class="pl-s1">session</span>.<span class="pl-en">merge</span>(<span class="pl-s1">dashboard</span>)
      <span class="pl-s1">db</span>.<span class="pl-s1">session</span>.<span class="pl-en">commit</span>()
    <span class="pl-k">except</span> <span class="pl-v">SQLAlchemyError</span> <span class="pl-k">as</span> <span class="pl-s1">e</span>:
      <span class="pl-s1">logger</span>.<span class="pl-en">error</span>(<span class="pl-s">'Failed to update dashboard: ?'</span>, <span class="pl-s1">e</span>.<span class="pl-s1">message</span>)
      <span class="pl-s1">db</span>.<span class="pl-s1">session</span>.<span class="pl-en">rollback</span>()
      <span class="pl-k">return</span> <span class="pl-c1">False</span>
      
    <span class="pl-k">return</span> <span class="pl-c1">True</span></pre></div>
<h4 dir="auto">Proposed example package structure that follows the above principles:</h4>
<div class="snippet-clipboard-content notranslate position-relative overflow-auto" data-snippet-clipboard-copy-content="superset/dashboards/api.py # Contains FAB and Flask endpoints related to the public API
superset/dashboards/schemas.py # Contains marshmallow schemas used by both the api and view endpoints
superset/dashboards/boot.py # Contains initialization and configuration code for API endpoints, invoked by `create_app()`
superset/dashboards/views.py # Contains FAB and Flask endpoints related to page-refresh views.
superset/dashboards/dao.py
superset/dashboards/commands/add_user.py
superset/dashboards/commands/create.py
superset/dashboards/helpers.py
superset/charts/api.py
superset/charts/daos.py
superset/charts/boot.py
superset/charts/schemas.py
superset/charts/commands/add_to_dashboard.py
superset/charts/commands/create.py
superset/charts/helpers.py
superset/models/chart.py # Models are thin SQLAlchemy objects - they do not implement custom queries or filters. That functionality will be found in the DAO, which manipulates the underlying SQLAlchemy model.
superset/models/dashboard.py"><pre class="notranslate"><code class="notranslate">superset/dashboards/api.py # Contains FAB and Flask endpoints related to the public API
superset/dashboards/schemas.py # Contains marshmallow schemas used by both the api and view endpoints
superset/dashboards/boot.py # Contains initialization and configuration code for API endpoints, invoked by `create_app()`
superset/dashboards/views.py # Contains FAB and Flask endpoints related to page-refresh views.
superset/dashboards/dao.py
superset/dashboards/commands/add_user.py
superset/dashboards/commands/create.py
superset/dashboards/helpers.py
superset/charts/api.py
superset/charts/daos.py
superset/charts/boot.py
superset/charts/schemas.py
superset/charts/commands/add_to_dashboard.py
superset/charts/commands/create.py
superset/charts/helpers.py
superset/models/chart.py # Models are thin SQLAlchemy objects - they do not implement custom queries or filters. That functionality will be found in the DAO, which manipulates the underlying SQLAlchemy model.
superset/models/dashboard.py
</code></pre></div>
<p dir="auto">In this design, all systems related to a specific back-end resource have been grouped under a top-level folder. <code class="notranslate">__init__.py</code> files should be left empty to enable only pulling in the portions of the system necessary for a specific entrypoint (Celery shouldn’t need <code class="notranslate">api.py</code> or <code class="notranslate">views.py</code> for instance)</p>
<h3 dir="auto">New or Changed Public Interfaces</h3>
<p dir="auto">Over time, the internals of Superset will evolve towards the new structure. Public HTTP interfaces will not be likely to change as a result of the above proposal, but code will move and alter to conform. This will impact organizations that apply their own customizations to Superset.</p>
<h3 dir="auto">New dependencies</h3>
<p dir="auto">None</p>
<h3 dir="auto">Migration Plan and Compatibility</h3>
<p dir="auto">Introduce refactors to existing code at a manageable pace to allow organizations relying on Superset internals time to adapt.</p>
<h3 dir="auto">Rejected Alternatives</h3>
<p dir="auto">Preset discussed Service Objects as an alternative to both Commands and DAOs. We felt that Commands provided easier entrypoints for the ports of our application (API endpoints, views, command line invocations, Celery tasks) than Service Objects, and that introducing DAOs as well helped further break down concerns.</p>
<p dir="auto">We also considered structuring top-level folders by function (api, models, etc.) but found this resulted in drastically more Python modules overall without substantially simplifying the question of where code should live.</p>
<h3 dir="auto">Individuals consulted in creating this SIP</h3>
<p dir="auto"><a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/mistercrunch/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/mistercrunch">@mistercrunch</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/craig-rueda/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/craig-rueda">@craig-rueda</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/dpgaspar/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/dpgaspar">@dpgaspar</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/robdiciuccio/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/robdiciuccio">@robdiciuccio</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/suddjian/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/suddjian">@suddjian</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/nytai/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/nytai">@nytai</a></p>