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

CRUD for backend api configs detached from proxy config page #1230

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Sep 23, 2019

@guicassolato guicassolato self-assigned this Sep 23, 2019
@guicassolato guicassolato marked this pull request as ready for review September 23, 2019 16:16
@Martouta Martouta requested a review from a team September 23, 2019 16:59
duduribeiro
duduribeiro previously approved these changes Sep 23, 2019
Copy link
Contributor

@duduribeiro duduribeiro left a comment

Choose a reason for hiding this comment

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

Looks good to me @guicassolato

🎖

Copy link
Member

@thomasmaas thomasmaas left a comment

Choose a reason for hiding this comment

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

Let's stick with backends. i understand that it's not the backend self you edit but I find Backend Configuration confusing.

@@ -0,0 +1,24 @@
h2 Backend API Configs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
h2 Backend API Configs
h2 Backends

Copy link
Member

Choose a reason for hiding this comment

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

we should stick to Products and Backends

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that we already have the term "Backend API" all over in the UI. Besides, "Backend" can be confusing. Apart from the obvious confusion with 3scale's backend component, the word itself can mean many different things depending on your perspective. (Like "upstream". Ask 10 people what they understand by "upstream" and you will get 11 answers.)

We should also remember that technically "Backend API" and "Backend API Config" are not the same thing. While one refers to an actual real API that exists independently of the API management layer, the other is an abstraction created to represent a specific configuration given to an API once in the context of a product. That same API can have a different config under a different product and it's not even forbidden to add the same API multiple times to the same product (with different configs). By calling two different concepts by the same name we may be seeding communication problems for the future.

Copy link
Member

Choose a reason for hiding this comment

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

You create backends/backend APIs, you create products/product APIs and then you use those backends/backend APIs in products product APIs.

I just created a backend/backend API, now I want to use that backend/backend API in a product but suddenly I have to add a backend API config to a product? That doesn’t make sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In formal database terminology, the backend-api-config is what it’s called a weak entity, it does not exist other than through the relationship between a product and a backend-api. Its ID means nothing as it will essentially be identified by the product’s ID and the backend-api’s ID that hold things together.

That should be enough to try to place the actual backend-api-config in the UI as something that is not independent of its stronger entities, but subtly representing the action, the movement, of adding a backend-api to a product.

These are therefore arguments in favor of not using the term “backend-config” in the UI:

  1. It’s not a strong entity
  2. It’s more an action then a thing

There’s one little thing that starts complicating things though. This “action” has proper information on its own other than just the IDs of its stronger entities. I’m talking about the “path” that today is the only “config” added for real to the picture, but it could one day become more than just that, or maybe not – who knows? Well, we know that “path” could easily become “path or header” very soon. Consider that in the end this is a proxy routing policy and whatever this type of policy admits may be one day decided to be added to the “backend-api-config”.

Yet, I agree that even as being a complex config it does not necessarily mean that we need to name it publicly. It’s the risk of giving the wrong notion that a backend-api is a child of a product that concerns me most. Imagine a costumer saying “I have these backend-apis X and Y here and want to do something whatever with them”. In 100% of the cases, the interlocutor will have to first give a step back and make sure to put things in the right context: “Are you talking about the actual backend-api or about a backend-api in a product?”. This “in a product” makes all the difference.

Another practicality to have in mind is how to name 3scale API endpoints. For sure we’ll have /services (maybe aliased /products) and /backend-apis. How will we introduce the “configs”? /services/:service_id/backend-apis? Does it reinforce the idea of backend-api as child of product?

If the pros and cons discussed here are clear, then we should have enough to decide about “backend-api-config” or just “backend-api” under a product. We solve the “-config” part.

Now let’s talk about the “-api” part. Without that, we’d have simply “backend”. IMO, nothing is more given in API management than the definition of API. It’s about adding functionality onto your APIs. Before start using 3scale, our customers already have APIs. Productizing, monetizing, analyzing, rate limiting are things the customer wants to do with its existing APIs.

Today in 3scale customers may not realize the difference between backend API and API with management layer added on top if it because it has always been a 1-to-1 relationship. APIAP is breaking that.

So I ask: from the two parts that remains once breaking current tight 3scale “service” into what you sell (product) and what you consume (source), what do we want? Should...
a) both be treated as “API”?
b) only what’s in the “front”, the “facade”, in the selling end?
c) only what’s behind, in the “backend”, what remains without the API management layer?
d) neither?

Finally, something easy to agree is that “backend” is already a troubling word for us. In general, it can be a backend of anything, including for example the “backend administration of API products”. But in 3scale, it can also mean the “backend” as in “the backend (apisonator) behind system”.

I know I only exposed questions and, honestly, it’s not for me to decide. But it’s important to be clear about what’s at stake. Did I miss anything?

So... “backend-api” or just “backend”? “backend(-api)-config” or just “backend(-api)”?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing that I missed... We can also use shorter terms for the menus and API URLs and longer ones for the titles and descriptions of the pages and endpoints.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @guicassolato. My 2 cents

Backend (API) vs Backend (API) Configuration

  1. Adding a Backend to a Product is more than just creating a Backend Configuration. It's the concept of linking 2 entities and make them work smartly together. At this moment that means adding a piece of configuration for APIcast but it will mean more in future iterations.

  2. Once a backend has been added to a Product we do need to make clear that editing its usage is not the same as editing the Backend itself.

For reason 1. I think we should keep Integration > Backends in vertical navigation and keep the action called Add Backend within Product scope. But for reason 2. the action to change the way the backend is used in this product (change path at this moment in time) should make it clear in the form that what you are editing is the usage of this backend in this specific product. For now I think we can get away with a hint underneath the path field along the lines of "Changing this path will only affect the behaviour of this Backend within the context of this Product".

Product API vs Product and Backend API vs Backend

The core of API as a Product is that our customers think of their APIs as potential Products. Products, maybe API Products but definitely not Product APIs.

At a technical level Backends and Products together become functional APIs that our customers customers can use.

ui

Our Dashboard says APIs and then splits them out in Products and Backends. And that's what we can call them all over the ui. No need to keep repeating API all over the place, it doesn't add any understanding. If this turns out to be a mistake after user testing we can change.

API

Our API says Service at this moment. In my opinion we can just add Backend and later rename Service to Product. (I would like them to have been in the APIs name space so endpoints become: apis/backends but give we already have a namespace admin/api, admin/api/apis/backends would be confusing.)

footnotes

I know backend is not an ideal name but I think in the end that will not really matter. If the feature is understood and useful people won't mind the naming.

From our customer perspective the backend is an API, From our customers customers perspective the product gives them an API. And in 3scale a Product together with 1 or more backend is a managed API. They are all APIs. Adding that word doesn't add any meaning imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

For API I propose for now (for ER1): /admin/api/services/:service_id/backend_apis
The other option for ER1 is: /admin/api/services/:service_id/backend_api_configs

Copy link
Member

Choose a reason for hiding this comment

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

Let me add that I understand we have already quite some API endpoints with backend_api merged and that QE might be writing automation on top of it. lets not spend any time on that before ER1 and keep with this naming for now (and very possibly forever). @Martouta

table#backend_api_configs.data
thead
tr
th = sortable('backend_apis.name', 'Backend API')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
th = sortable('backend_apis.name', 'Backend API')
th = sortable('backend_apis.name', 'Backend')

th = sortable('backend_apis.private_endpoint', 'Private URL')
th = sortable('backend_api_configs.path', 'Path')
th.actions
= action_link_to :add, new_admin_service_backend_api_config_path(@service), label: 'Add Backend API'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
= action_link_to :add, new_admin_service_backend_api_config_path(@service), label: 'Add Backend API'
= action_link_to :add, new_admin_service_backend_api_config_path(@service), label: 'Add Backend'

When I follow "Overview"
And I follow "Integration" within the main menu
Then I should see menu items
| Configuration |
| Methods & Metrics |
| Mapping Rules |
| Settings |
| Backend APIs |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| Backend APIs |
| Backends |

items << {id: :methods_metrics, title: 'Methods & Metrics', path: admin_service_metrics_path(@service)}
items << {id: :mapping_rules, title: 'Mapping Rules', path: admin_service_proxy_rules_path(@service)} if current_account.independent_mapping_rules_enabled?
if current_account.provider_can_use?(:api_as_product)
items << {id: :backend_api_configs, title: 'Backend APIs', path: admin_service_backend_api_configs_path(@service)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
items << {id: :backend_api_configs, title: 'Backend APIs', path: admin_service_backend_api_configs_path(@service)}
items << {id: :backend_api_configs, title: 'Backends', path: admin_service_backend_api_configs_path(@service)}

collection: current_account.backend_apis,
include_blank: false,
prompt: 'Select a Backend API',
label: 'Backend API',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
label: 'Backend API',
label: 'Backend',

@@ -0,0 +1,4 @@
= content_for :sublayout_title, 'Edit Backend API'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
= content_for :sublayout_title, 'Edit Backend API'
= content_for :sublayout_title, 'Edit Backend'

Copy link
Member

Choose a reason for hiding this comment

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

mmm, you don't edit the backend. Not sure, maybe we should call them Backend Integrations but lets just stick with backends for now.

Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

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

This is dangerous 😞

@@ -34,7 +63,11 @@ def authorize

delegate :backend_api_configs, to: :service

def backend_api_params
def backend_api_config_params
params.require(:backend_api_config).permit(:backend_api_id, :path)
Copy link
Contributor

@Martouta Martouta Sep 24, 2019

Choose a reason for hiding this comment

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

I find this very dangerous... This accepts any existing backend_api_id even if that backend_api does not belong to the same tenant.
This is why I did and said this: #1231 (comment)
A safer approach for example could be:

def backend_api_params
  backend_api_params = params.require(:backend_api_config).permit(:backend_api_id, :path)
  if (backend_api_id = backend_api_params.delete(:backend_api_id))
    backend_api_params[:backend_api] = current_account.backend_apis.find(backend_api_id)
  end
  backend_api_params
end

Or:

def backend_api_params
  params.require(:backend_api_config).permit(:backend_api_id, :path).tap do |backend_api_params|
  if (backend_api_id = backend_api_params.delete(:backend_api_id))
    backend_api_params[:backend_api] = current_account.backend_apis.find(backend_api_id)
  end
end

And of course no matter how you solve this issue, the fact of solving it requires a test because imo it is a big issue and easy to forget 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, it is also better to return a 404 when it does not exist at all than a DB error raised for the foreign key being invalid 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes loading the object for 2 cases:

  • it exists
  • it belongs to the same tenant

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #1230 into master will decrease coverage by 0.75%.
The diff coverage is 97.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1230      +/-   ##
==========================================
- Coverage   93.14%   92.38%   -0.76%     
==========================================
  Files        2485     2403      -82     
  Lines       82360    78242    -4118     
==========================================
- Hits        76713    72284    -4429     
- Misses       5647     5958     +311
Impacted Files Coverage Δ
app/helpers/vertical_nav_helper.rb 95.55% <100%> (+0.04%) ⬆️
config/routes.rb 98.06% <100%> (ø) ⬆️
app/models/backend_api_config.rb 100% <100%> (ø) ⬆️
...gration/api/backend_api_configs_controller_test.rb 100% <100%> (ø) ⬆️
.../controllers/api/backend_api_configs_controller.rb 95% <91.3%> (-5%) ⬇️
app/queries/auto_account_deletion_queries.rb 40% <0%> (-60%) ⬇️
app/lib/events/importers/first_traffic_importer.rb 40% <0%> (-60%) ⬇️
app/workers/purge_old_user_sessions_worker.rb 40% <0%> (-60%) ⬇️
app/helpers/google_experiments_helper.rb 28.57% <0%> (-57.15%) ⬇️
.../initializers/backport_pg_10_support_to_rails_4.rb 30% <0%> (-50%) ⬇️
... and 150 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c779e62...137b2b8. Read the comment docs.

@guicassolato guicassolato force-pushed the apiap-independent-backend-api-configs branch from 5f2b770 to 137b2b8 Compare September 25, 2019 08:45
Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

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

Awesome 💪

@thomasmaas thomasmaas merged commit c5d78eb into master Sep 25, 2019
@thomasmaas thomasmaas deleted the apiap-independent-backend-api-configs branch September 25, 2019 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants