[backend-api] API Product export

[simara-svatopluk]

Q	A
Description, reason for the PR	We add support for backend API and exporting products in the very moment
New feature	Yes
BC breaks	No
Fixes issues	...
Have you read and signed our License Agreement for contributions?	Yes

[grossmannmartin]

Hi, Thanks for the PR. I like the idea 👍 and the separation is very nice.

[henzigo]

Hello, I have some points.

1. I would be glad if that bundle would not be as part as project-base by default. Only some projects will use this bundle so why others should have it installed too? For me, project-base should be simplest as it can be. You can just make it an independent module with documentation on how to install that.
2. Your new bundle is named as ApiBundle, so if you will create FE API in the future, you will place it here or you will create a new bundle? If you select the second option I suggest naming this bundle as BackendApiBundle :)

[simara-svatopluk]

@LukasHeinz @PetrHeinz Hi, #1055 (comment) suggest that API won't be a default part of project-base and implementator will have to always install it. It makes sense to me because the project-base will be more lightweight by default.

On the other hand it would be a first package that is not part of the project-base so I expect unknown amount of troubles. Troubles that comes to my mind right now are complications during testing and continuous integration (we would maybe need a special job that installs the API and it and it would take time to maintain such job). We'll also have to provide special install instructions. It will complicate automatic testing other than unit testing (smoke, functional, performance, acceptance).

So not having API in default project-base will add a lot of effort in my opinion. On the other hand it is a step in the right way in my opinion (lightweight SSFW).
I'm also thinking about separating the API from default project-base "in the future"... It will add even more effort in my opinion and if we don't separate it now, we'll simply never do it.

[PetrHeinz]

@simara-svatopluk I think not having the API registered by default is a step in right direction and I fully support it 👍

I think that being able to test the Shopsys Framework in different configurations will be essential in the near future. I don't see any benefit in postponing it.

As a side note - maybe we'd want to remove product feeds from the default shopsys/project-base repository as well (for the same reasons).

[LukasHeinz]

@simara-svatopluk From my point of view, I would not like to have different configurations of SSFW unless we have automatic tests for several configurations.

I recommend to have it as dependency on project-base and when we improve our quality assurance process and add automatic tests for the installation of project-base in different setups. It is planned within the next few months.

Do you see any disadvantage in postponing it until we have improved infrastructure?

[henzigo]

I'm sorry. I miss clicked that.

[simara-svatopluk]

I'm sorry. I miss clicked that.

@henzigo I disagree.... closing :-D Friday 🐼

[PetrHeinz]

@LukasHeinz - I would like to point out two disadvantages: confusing upgrade notes and the risk of postponing it for too long...

[henzigo]

And removing database fields which were migrated with API bundle is not that easy.

[LukasHeinz]

I have been informed, that the automatic test of these two configs will be easy. So for me - if the test of the new configuration is part of this new feature (not necessary part of the same PR, but will be deployed at theč same time), I am okay with separation of PB and API.

[TomasLudvik]

I like this style of docs, good job @vitek-rostislav

[simara-svatopluk]

@henzigo We've just decided that for now the API will be part of project-base and it will be merged to 8.0 master. The separation will be done before the 8.0 is released and @PetrHeinz commited to push this separation.

We decided for this approach because this API PR have already taken a lots of effort (and also will take), and we believe that "have something done" is better than "have something in progress for a long time".

[TomasLudvik]

Hi guys!
I did some testing and here are the results (something is in the code itself and general ideas are here).

1. backend-api-oauth-keys-generate is called before composer and on a first try, it left me wondering what is happening. I needed to run composer update manually even though it's called in phing targets.
2. build of the application failed on db migrations because method uuid_generate_v4() was not found in PostgreSQL. I had to run db-create manually and several tries later I realized I had to call test-db-create too. It would be nice to have this mentioned somehow in ugprade notes. And think about already running production shops too (there, you can't just call db-create or can you?). Maybe some info about needed extensions for postgre in application requirements would be nice.
3. Product response returns names associated with locales, but descriptions (long and short) with domains

{
    "name": {
        "en": "name"
    },
    "shortDescription": {
        "1":  "short description"
    }
}

4. In introduction article I miss links to other articles in the same folder (versioning, methods). I think it helps with "continuity of documentation"
5. Postman didn't work properly when I set authorization on the Authorization tab and I needed to set it directly as a Header. I mention this only because you recommend Postman in documentation and maybe it's just problem on my machine. If you could try it, it would be nice.
6. Method api/token accepts POST and GET even though the documentation says only POST is accepted.
7. api/token is not versioned? What if we decide to change it to a different token type (JWT)? Could it be a problem?
8. API calls with bad token returns 401 with "The resource server rejected the request." response, but calling without the authorization header returns 401 without response. I believe it should behave the same way and returns json response (same way as calling incorrect endpoint do – api/v1/articles)
9. getProducts with page number larger than the page number of the last page shows the last page instead of 404. I'm not sure if it's intended.
10. According to https://restfulapi.net/http-methods/ (Summary of HTTP methods), the invalid UUID should return 404, not 500.
11. getProducts/UUID on non-existing UUID returns 404 without any response. I would like to have the same response as calling incorrect endpoint do – api/v1/articles
12. getProducts (as list) with all non-existing UUID(s) returns 200 and empty "array". I don't know if it's ok.
13. getProducts (as list) with combination of malformed UUID and a correct UUID returns 500 response. I think it should return valid responses same way as a combination of non-existing uuid and correct uuid do.

Hello @grossmannmartin, thank you so much for testing this!

1. In our upgrade instructions (https://github.com/shopsys/shopsys/blob/master/UPGRADE.md#you-are-developing-a-project-based-on-the-project-base-repository) and even in install script there is requirement to run composer install / update before running build of application. But I hear you and I have moved the target execution after composer.
2. I have added last but one upgrade instruction to run db-create and test-db-create
3. Well that is how Shopsys Framework is made, you have some suggestion?
4. I have added links to those articles at the end of introduction.
5. this works?
6. The trikorder/oauth2 bundle support multiple grant types for token generation, we decided to use the clients_credentials type and this is supported via POST.
7. Well I do not suppose this if this was a problem in future we might use another url like /api/token-jwt ...
8. Well this one is directed via trikorder/oauth-bundle and it decides this error response. I have removed content from api-methods.md so there is no confusion why there is no response when there is supposed to be.
9. Yes it is. Endpoints that returns list of Resources are supposed to always return 200 and array of Resources that were filtered. In your case there were no products found for your filtration criteria so you got empty array.
10. The documentation says:

if it is determined that GET request itself is not correctly formed then server will return HTTP response code 400 (BAD REQUEST).
I have added new exception if there is not valid UUID.

11. Done
12. That is okay
13. Changed to 400

[simara-svatopluk]

3. Well that is how Shopsys Framework is made, you have some suggestion?

3. Product response returns names associated with locales, but descriptions (long and short) with domains

This is ehm... not so informative replay. How could Martin have any suggestion? It doesn't make any sense...
Product have names for different locales, so en key is correct. Product descriptions (as well as SEO meta) are defined for domains, so the key has to be 1. This is because you could have 2 czech shops on different domains selling the same product. And once you need different description for each shop (common requirement), then you have to distinguish descriptions by domain, not locale

[simara-svatopluk]

6. Method api/token accepts POST and GET even though the documentation says only POST is accepted.

6. The trikorder/oauth2 bundle support multiple grant types for token generation, we decided to use the clients_credentials type and this is supported via POST.

At first it's strange it works with GET. HTTP GET should not contain the message body (well, it is not restricted, so it potentionally could), but what is more important server shouldn't process the GET message body. So in this case the server should reply with an error "no data". But to be honest, I personally don't consider to be a big problem... do you think this is a problem that has to be addressed?

[grossmannmartin]

Thanks for the fixes :)

1. Thank you!
2. Awesome
3. Okay, I understand the reasons. I didn't realize descriptions are multi-domain and name is multi-language attribute. As it may seem weird to me, it's exactly how we work with these attributes. 👍
4. 👍
5. I updated postman and it's ok. Interesting. Okay
6. I don't see this as a big problem. I notice strange behavior and need to point it out. But I don't understand the HTTP protocol deep enough to see if it can be somehow misused. If it's not easy to disallow GET method, I'm ok with a current state.
7. Okay... I don't have any arguments against it.
8. Well, okay. If it's not possible to make it the same way as other error states, I can live with that. But I would like to have every "error" states with JSON response.
9. I think you respond to something other than I wrote.
10. Better!
11. 👍
12. 👍
13. Malformed request. Okay 👍

TLDR:
please take a look at 9 and consider if 8 is really impossible to do (I really don't know)

[simara-svatopluk]

@grossmannmartin
The problem is that the text response is send by the OAuth2 bundle (in case of 401), and I didn't find a quick way to change it to JSON. I tried to check what the OAuth2 standard says about response, but it is vague... there is something like "the server could provide additional information", but the important is that it returns 401.

I could spend time on this issue, but honestly I don't see big added value (as an implementator I would only check the response code) and it would also take time and this PR won't be done today. I know the second argument is forcing... but it's the current state.

After this reply... what do you think? Do we have to fix it right now?

[TomasLudvik]

@grossmannmartin,
9) It will now return 422 There are no products on provided page.

[grossmannmartin]

9. Godlike!

[grossmannmartin]

@simara-svatopluk: Okay, I understand and I'm ok with current state

[LukasHeinz]

Nice job guys! Really. The docs are quite complete and easy to understand and read. Nevertheless, I just found something in docs, but only minor things.