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

docs(specs): review Insights API spec #1647

Merged
merged 29 commits into from
Jun 22, 2023
Merged

docs(specs): review Insights API spec #1647

merged 29 commits into from
Jun 22, 2023

Conversation

kai687
Copy link
Contributor

@kai687 kai687 commented Jun 20, 2023

🧭 What and Why

Review and refine the OpenAPI specification for the Insights API.

🎟 JIRA Ticket: DOC-1110

Changes included:

  • Correct parameter limits (maxItems for array types, maxLength for strings)
  • Updated API, operation, and parameter descriptions and summaries
  • Correct responses
  • Example requests and responses, particularly errors
  • Separate request bodies for allowed combinations of event attributes

🧪 Test

@netlify
Copy link

netlify bot commented Jun 20, 2023

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit 68482bd
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/649457efe0fec80008de9ca4
😎 Deploy Preview https://deploy-preview-1647--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Jun 20, 2023

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

@kai687 kai687 requested a review from gazconroy June 20, 2023 17:01
@kai687
Copy link
Contributor Author

kai687 commented Jun 20, 2023

@shortcuts I exchanged anyOf with oneOf and most checks work, except some tests. oneOf is the correct semantics here, since any event can only match one schema exactly.

What do you think about this?

Copy link
Contributor

@gazconroy gazconroy left a comment

Choose a reason for hiding this comment

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

Some questions and tiny suggestions

Co-authored-by: gazconroy <gazconroyster@gmail.com>
@kai687 kai687 marked this pull request as ready for review June 21, 2023 09:55
@kai687 kai687 requested a review from a team as a code owner June 21, 2023 09:55
@kai687 kai687 requested review from damcou and shortcuts June 21, 2023 09:55
@kai687 kai687 requested a review from gazconroy June 21, 2023 09:55
@shortcuts
Copy link
Member

@shortcuts I exchanged anyOf with oneOf and most checks work, except some tests. oneOf is the correct semantics here, since any event can only match one schema exactly.

What do you think about this?

This looks correct indeed, I wonder why this test doesn't work for Java. Could you double check properties properly match their type etc.?

@kai687
Copy link
Contributor Author

kai687 commented Jun 21, 2023

@shortcuts This is why the test fails:

 ClickedObjectIDsAfterSearch events_12 = new ClickedObjectIDsAfterSearch();
        {
          ClickEvent eventType3 = ClickEvent.fromValue("view");
          events_12.setEventType(eventType3);
          String eventName3 = "Product Detail Page Viewed";
          events_12.setEventName(eventName3);
          String index3 = "products";
          events_12.setIndex(index3);
          String userToken3 = "user-123456";
          events_12.setUserToken(userToken3);
          long timestamp3 = 1641290601962L;
          events_12.setTimestamp(timestamp3);
          List<String> objectIDs3 = new ArrayList<>();
          {
            String objectIDs_04 = "9780545139700";
            objectIDs3.add(objectIDs_04);
            String objectIDs_14 = "9780439784542";
            objectIDs3.add(objectIDs_14);
          }
          events_12.setObjectIDs(objectIDs3);
        }

It's a view event, but it uses the request body for ClickedObjectIDsAfterSearch. It should use ViewedObjectIDs instead (likewise for the conversion test).

@shortcuts
Copy link
Member

@shortcuts This is why the test fails:

 ClickedObjectIDsAfterSearch events_12 = new ClickedObjectIDsAfterSearch();
        {
          ClickEvent eventType3 = ClickEvent.fromValue("view");
          events_12.setEventType(eventType3);
          String eventName3 = "Product Detail Page Viewed";
          events_12.setEventName(eventName3);
          String index3 = "products";
          events_12.setIndex(index3);
          String userToken3 = "user-123456";
          events_12.setUserToken(userToken3);
          long timestamp3 = 1641290601962L;
          events_12.setTimestamp(timestamp3);
          List<String> objectIDs3 = new ArrayList<>();
          {
            String objectIDs_04 = "9780545139700";
            objectIDs3.add(objectIDs_04);
            String objectIDs_14 = "9780439784542";
            objectIDs3.add(objectIDs_14);
          }
          events_12.setObjectIDs(objectIDs3);
        }

It's a view event, but it uses the request body for ClickedObjectIDsAfterSearch. It should use ViewedObjectIDs instead (likewise for the conversion test).

Ah yessss there's an issue with the oneOf in the CTS that will pick the first oneOf type available if it can't correlate it correctly, cc @millotp

@shortcuts
Copy link
Member

Ok so I can confirm this is definitely a test generation issue, I'll push a fix to your branch sorry for the blocker

@shortcuts
Copy link
Member

#1654 fixes the CTS generation for this PR

gazconroy
gazconroy previously approved these changes Jun 22, 2023
@shortcuts
Copy link
Member

so sadly #1654 fixes this PR issue but would create a new problem :( so I just removed the test for now and will investigate

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Huge! It's kind of a breaking change but it's the best time to provide them :D

thanks for a lot for the review

@shortcuts shortcuts requested a review from gazconroy June 22, 2023 14:48
@shortcuts shortcuts merged commit b703dea into main Jun 22, 2023
@shortcuts shortcuts deleted the feat/insights-oneof branch June 22, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants