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

feat(java): replace EchoRequester by Interceptor APIC-522 #648

Merged
merged 4 commits into from
Jun 8, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Jun 7, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-522

Remove the obsolete EchoRequester and replace it with the much better EchoInterceptor, allowing access to the data after the retry strategy, like the host.

🧪 Test

CI

@millotp millotp self-assigned this Jun 7, 2022
@netlify
Copy link

netlify bot commented Jun 7, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 4ed6626
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/62a066f94985fd00086ffa7f

@millotp millotp requested review from a team, damcou and shortcuts and removed request for a team June 7, 2022 17:36
@algolia-bot
Copy link
Collaborator

algolia-bot commented Jun 7, 2022

✗ 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.

damcou
damcou previously approved these changes Jun 8, 2022
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Looks very nice and useful !
So if I understand correctly, the whole purpose to switch to EchoInterceptor was to be able to run all the tests and you only use it there ?

Not sure why the CI breaks though

@millotp
Copy link
Collaborator Author

millotp commented Jun 8, 2022

Moving the echo requester further allows us to test the retry strategy too, which was not possible before, and we can add more test for this, now it's pretty light

@@ -11,7 +11,7 @@
},
"expected": {
"type": "userAgent",
"match": "^Algolia for ${{languageCased}} \\(\\d+\\.\\d+\\.\\d+\\)(; [a-zA-Z. ]+ (\\(\\d+\\.\\d+\\.\\d+\\))?)*(; ${{clientPascalCase}} (\\(\\d+\\.\\d+\\.\\d+\\)))(; [a-zA-Z. ]+ (\\(\\d+\\.\\d+\\.\\d+\\))?)*$"
"match": "^Algolia for ${{languageCased}} \\(\\d+\\.\\d+\\.\\d+(-.*)?\\)(; [a-zA-Z. ]+ (\\(\\d+\\.\\d+\\.\\d+(-.*)?\\))?)*(; ${{clientPascalCase}} (\\(\\d+\\.\\d+\\.\\d+(-.*)?\\)))(; [a-zA-Z. ]+ (\\(\\d+\\.\\d+\\.\\d+(-.*)?\\))?)*$"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep but it's closed now ahah

damcou
damcou previously approved these changes Jun 8, 2022
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

🚀

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.

Nice! Small comments about docs

tests/CTS/client/common/commonApi.json Show resolved Hide resolved
@@ -0,0 +1,91 @@
package com.algolia;
Copy link
Member

Choose a reason for hiding this comment

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

(We mention echoRequester for all clients in #564, we will need to update this part for client specific)

^Algolia for <LANGUAGE> \\(\\d+\\.\\d+\\.\\d+(-.*)?\\)(; [a-zA-Z. ]+ (\\(\\d+\\.\\d+\\.\\d+(-.*)?\\))?)*(; <CLIENT> (\\(\\d+\\.\\d+\\.\\d+(-.*)?\\)))(; [a-zA-Z. ]+ (\\(\\d+\\.\\d+\\.\\d+(-.*)?\\))?)*$
```

The function MUST be named `addAlgoliaAgent` because of JavaScript exception that doesn't allow custom `User-Agent` in the header, and must use `x-algolia-agent` for JavaScript.
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing but I don't know how to word it better, actually only JS is named addAlgoliaAgent, because we allow appending to the client UA

In other clients, it's setAlgoliaAgent because it overrides everything, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it's not the same behavior, in js you can never override the user agent it seems like, only add segment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yes you can pass a custom header directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will remove setAlgoliaAgent in the next PR for java and php

client: `; Search (5.0.0)`
segment: `; JVM (11.0.14); experimental`
```
- base: `Algolia for Java (4.0.0)`
Copy link
Member

Choose a reason for hiding this comment

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

For this example it should follow the shape of the result so it's base -> segment -> client for Java

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it depends in which order you add the segments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and this is the actual user agent returned by the client

Copy link
Member

Choose a reason for hiding this comment

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

Yes but in your example client is added before segments but the output states JVM in second

Copy link
Member

Choose a reason for hiding this comment

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

Ah you consider the example is using the Java client, globally reading (without knowledge of clients), it feels like it's magically reordered but I don't think we mind

Copy link
Collaborator Author

@millotp millotp Jun 8, 2022

Choose a reason for hiding this comment

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

Maybe we should differenciate internal segments and user defined segments, but this is a confusion we don't need to add in this doc.
JVM is an internal segment, and will be added at the creation, then Search is added, and finally the user defined segments are added.

scripts/ci/githubActions/setRunVariables.ts Show resolved Hide resolved
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.

Looks good!!

@millotp millotp merged commit d630514 into main Jun 8, 2022
@millotp millotp deleted the feat/java-echo-interceptor branch June 8, 2022 09:21
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.

4 participants