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

upgrade test container & more overridenUrl settings? #64

Closed
qyb opened this issue Jun 15, 2022 · 6 comments
Closed

upgrade test container & more overridenUrl settings? #64

qyb opened this issue Jun 15, 2022 · 6 comments

Comments

@qyb
Copy link
Contributor

qyb commented Jun 15, 2022

First, James 3.7.0 have implemented Thread/Get (JAMES-3516). It's an important feature and I'd like to push my patch that had been working well with cyrus-imapd.

Second, current constructor has an optional overriddenApiUrl argument. I thought we need it when the real James server serves behind a proxy with rewritten path/port (or our test container's port mapping ). But apiUrl wasn't the only URL in jmap session response. downloadUrl, uploadUrl, eventSourceUrl, ... all need to be replaced in this scenario. Should we use overriddenUrl-object instead of it?

@qyb
Copy link
Contributor Author

qyb commented Jun 16, 2022

Now I get the answers.

First, James 3.7.0 have implemented Thread/Get (JAMES-3516). It's an important feature and I'd like to push my patch that had been working well with cyrus-imapd.

linagora/tmail-backend:memory-0.5.0 is the first 3.8.0-SNAPSHOT image of tmail-backend, and 0.4.3 is the last 3.7.0-SNAPSHOT image. I'll use 0.5.0 as my test container.

Second, current constructor has an optional overriddenApiUrl argument. I thought we need it when the real James server serves behind a proxy with rewritten path/port (or our test container's port mapping ). But apiUrl wasn't the only URL in jmap session response. downloadUrl, uploadUrl, eventSourceUrl, ... all need to be replaced in this scenario. Should we use overriddenUrl-object instead of it?

If set dynamic.jmap.prefix.resolution.enabled=true in jmap.properties. Then we could use X-JMAP-PREFIX header to get usable URLs in session request.

So we just need one prefixUrl argument instead of overriddenApiUrl .

@chibenwa
Copy link
Member

linagora/tmail-backend:memory-0.5.0 is the first 3.8.0-SNAPSHOT image of tmail-backend, and 0.4.3 is the last 3.7.0-SNAPSHOT image. I'll use 0.5.0 as my test container.

Why not use apache James 3.7.0 ? https://hub.docker.com/layers/james/apache/james/memory-3.7.0/images/sha256-944c7067ec87b262b0ccb8a50204a61058ed6f2033e9dff9323db930cba2618a?context=explore

Second, current constructor has an optional overriddenApiUrl argument. I thought we need it when the real James server serves behind a proxy with rewritten path/port (or our test container's port mapping ). But apiUrl wasn't the only URL in jmap session response. downloadUrl, uploadUrl, eventSourceUrl, ... all need to be replaced in this scenario. Should we use overriddenUrl-object instead of it?

If set dynamic.jmap.prefix.resolution.enabled=true in jmap.properties. Then we could use X-JMAP-PREFIX header to get usable URLs in session request.

So we just need one prefixUrl argument instead of overriddenApiUrl .

X-JMAP-PREFIX was intended to be used by an API gateway.

The use case was exposing two APIs, one legacy using basic auth + a custom JWT mechanism while still having another set of API using OIDC, all of that collocated on the same james deployment: The API gateway then just injects the right values to populate the JMAP session. In short: a deployment detail.

To be fair, as a client you should not care. You should blidly take the endpoints advertized in the JMAP session provided by your backend and just use that. (any thing different would harm inter-operability)

@chibenwa
Copy link
Member

chibenwa commented Jun 16, 2022

(so I would rather see less overrides rather than more!)

@alagane
Copy link
Member

alagane commented Jun 16, 2022

The main reason to use overridenApiUrl is because the server (James) configuration would not be done correctly, for testing purpose.
If the server is configured correctly I think we might not need this parameter anymore.

@chibenwa
Copy link
Member

Ok so that option is IMO harmful and should be dropped.

@alagane
Copy link
Member

alagane commented Jun 21, 2022

Ok so that option is IMO harmful and should be dropped.

Created issue

#66

@qyb qyb closed this as completed Jun 21, 2022
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

No branches or pull requests

3 participants