ash-project / ash_json_api

The JSON:API extension for the Ash Framework
https://hexdocs.pm/ash_json_api
MIT License
56 stars 40 forks source link

Prefix is not appended to generated routes, only to the OpenAPI schema #107

Closed m0rt3nlund closed 10 months ago

m0rt3nlund commented 10 months ago

Describe the bug If one uses the prefix in Ash.Api this prefix is not included then the routes are generated, only for the routes in the schema

Maybe this line needs to also have some code to get the prefix for the route? https://github.com/ash-project/ash_json_api/blob/v0.33.1/lib/ash_json_api/api/router.ex#L72

To Reproduce

defmodule MyLib.Ash.ApiCode do
  use Ash.Api, extensions: [AshJsonApi.Api]

  resources do
    registry MyLib.Ash.ApiRegistry
  end

  json_api do
    prefix "/myprefix"
  end
end

Resource:

json_api do
    type "myresource"

    routes do
      base("/myresource")
      get(:read)
    end
  end

This would generate routes in OpenAPI to say "/myprefix/myresource" but the route in the "Router" will be "/myresource"

** Runtime

zachdaniel commented 10 months ago

prefix is not meant to be added to the routes, it is used when they have that prefix in the router. So if you used scope .... and added your routes inside of that, i.e

scope "/api/v1" do
  ...add json api router here
end

You'd use the prefix to tell us that you did that.

m0rt3nlund commented 10 months ago

Hi! That would make sense to me if I was not able to add more than one API into the apis property in AshJsonApi.Api.Router If you then set different prefix in the APIs you will no longer be able to generate the routes when the Router is located in ascope`?

What it is the argument against having AshJsonApi.Api.Router generate complete URL relative to the scope set for AshJsonApi.Api.Router based on the prefix in each AshJsonApi.Api extension?

This would make it very easy to make all resources in an Api have the same initial path. Instead now I have to set the prefix as part of the base path in each module.

I might be using it incorrectly?

To me it would be better if I could add multiple APIs in the router and that the router generated relative paths to the resources including the prefix in each Api.

router scope = /api/

prefix for API1 = /api/inventory base for Api1.Resource = /item

prefix for API2 = /api/accounts base for Api2.Resource = /user

and then this would automatically result in both OpenAPI docs and a route like this:

/api/inventory/item
/api/accounts/user

Or should I make multiple AshJsonApi.Api.Router and setup the Swagger frontend using the modify_open_api property to allow for multiple urls as this would be needed for the Swagger frontend to show the output from the other AshJsonApi.Api.Router ?

See the urls section here: https://github.com/swagger-api/swagger-ui/blob/master/docs/usage/configuration.md

zachdaniel commented 10 months ago

The essential issue is that one resource might appear in multiple apis, but you mount each api at some point in your router. The prefix is for you to tell us about where in the router you've mounted it so we can generate appropriate routes in other places. A better name might be something like router_prefix. I'd be open to adding another option that behaves the way that you're saying. Something like base_route? I see how that would be confusing, but for backwards compatibility reasons we can't really change the behavior of the existing option.

m0rt3nlund commented 9 months ago

Hi again! When I used the scope correctly this all made sense!

But, someone just implemented the changes in 0.34 where prefix is added to the route and one cannot use scopes anymore..

https://github.com/ash-project/ash_json_api/blob/v0.34.0/lib/ash_json_api/api/router.ex#L73

If you have the router inside a scope you would need to add the "prefix" in the API as you said and this will generate a correct Open API spec, but it will now result in a route that will have the prefix twice.

# Router
scope "/config" do
    pipe_through [:api]
    forward "/", ApiWeb.Public.Config
  end

# API
defmodule Shared.Ash.Config do
  use Ash.Api, extensions: [AshJsonApi.Api]

  resources do
    registry Shared.Ash.Config.Registry
  end

  json_api do
    prefix "/config"
    log_errors?(true)
  end
end

# Resource
json_api do
    type "node"

    routes do
      base("/node")

      get(:read)
      index(:paged, default_fields: [:name])
    end
  end

The code above should have generated routes like this: /config/node as you described. In the OpenAPI Spec it appears correct but it requires one to make requests to /config/config/node to access it now..

Maybe not intentional, or has there been other changes as well that I dont follow? :)

zachdaniel commented 9 months ago

Yep, this was a bug added recently. Can you try main? I believe it should be solved now.

m0rt3nlund commented 9 months ago

Ahh! I see!

I cannot depend on the repo at the moment:

# mix.exs
{:ash_json_api, git: "https://github.com/ash-project/ash_json_api.git"},

> mix deps.get
* Getting ash_json_api (https://github.com/ash-project/ash_json_api.git)
remote: Enumerating objects: 3495, done.        
remote: Counting objects: 100% (624/624), done.
remote: Compressing objects: 100% (323/323), done.        
remote: Total 3495 (delta 383), reused 494 (delta 300), pack-reused 2871        
https://github.com/ash-project/ash_json_api.gitorigin/HEAD set to main
error: invalid path 'documentation/dsls/DSL:-AshJsonApi.Api.cheatmd'
error: invalid path 'documentation/dsls/DSL:-AshJsonApi.Resource.cheatmd'

It works when I manually git clone, but not when using mix deps.get

zachdaniel commented 9 months ago

Should be fixed now?