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(@cubejs-client/core): allow making HTTP POST requests to Cube.js API #1608

Merged
merged 1 commit into from
Dec 18, 2020
Merged

feat(@cubejs-client/core): allow making HTTP POST requests to Cube.js API #1608

merged 1 commit into from
Dec 18, 2020

Conversation

mnifakram
Copy link
Contributor

@mnifakram mnifakram commented Dec 15, 2020

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

#1558

@mnifakram mnifakram requested review from a team as code owners December 15, 2020 20:04
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Dec 15, 2020
@mnifakram
Copy link
Contributor Author

@hassankhan @ovr can you please guys review? Thanks.

@hassankhan hassankhan linked an issue Dec 15, 2020 that may be closed by this pull request
@@ -2,34 +2,40 @@ import fetch from 'cross-fetch';
import 'url-search-params-polyfill';

class HttpTransport {
constructor({ authorization, apiUrl, headers = {}, credentials }) {
constructor({ authorization, apiUrl, method = 'GET', headers = {}, credentials }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ovr Should we also be changing the default to POST too?

Copy link
Member

Choose a reason for hiding this comment

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

We should prefer the GET method whenever possible

Copy link
Member

Choose a reason for hiding this comment

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

100% agree with @vasilev-alex, it's better to use GET whenever possible.

Copy link
Member

Choose a reason for hiding this comment

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

@vasilev-alex @hassankhan Maybe it's better to set up it to undefined by default and detects automatically.

method = method || (url.length < 2000 ? 'GET' : 'POST')

Copy link
Member

Choose a reason for hiding this comment

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

@ovr yep, let's do like this. @mnifakram can you please introduce this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sure thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ovr @vasilev-alex I've just updated it. Please kindly check it. thanks

@hassankhan hassankhan added the enhancement New feature proposal label Dec 15, 2020
@hassankhan hassankhan changed the title feat: Add HTTP Post to cubejs client core feat(@cubejs-client/core): allow making HTTP POST requests to Cube.js API Dec 15, 2020
@vasilev-alex
Copy link
Member

So, currently, I can see one limitation. It's not possible to change the method once we've created the cubejsApi instance.

const transport = new HttpTransport({
  // ...
  method: 'POST'
});

const cubejsApi = cubejs(CUBEJS_TOKEN, {
  apiUrl: `${API_URL}/cubejs-api/v1`,
  transport
});

// Can't change the `method` during runtime
// I want to send this one using `GET`
cubejsApi.load(query);
// and this one using `POST`
cubejsApi.load(query);

What if we would pass the HTTP method to the load?

cubejsApi.load(query, { method: 'GET' });
cubejsApi.load(query, { method: 'POST' });

Or, maybe, we could allow users to override the method by passing it to the load method? Do we even need it?

@ovr @hassankhan @mnifakram what do you think?

@mnifakram
Copy link
Contributor Author

@vasilev-alex I don't think that users will switch between GET or POST, that's an edge case. In real world as a user if I didn't encounter the large URL problem and I only have POST, I will not even complain about not having GET as an option.

I don't see any advantages of using GET over POST. Not to mention that, it is way easier to inspect the query in the request body vs trying to decoded from the request URL.

@ovr
Copy link
Member

ovr commented Dec 16, 2020

I don't see any advantages of using GET over POST.

GET requests support cache inside browser, and possible Cube.js will return specific headers to support it.

@mnifakram
Copy link
Contributor Author

Is there anything that I can help with to get this merged? I'm really in need to get this released asap. Thanks

@mnifakram mnifakram requested review from vasilev-alex and removed request for a team December 16, 2020 22:14
@himessa
Copy link

himessa commented Dec 18, 2020

Any update on this? I need the ability to handle large queries as well.

@vasilev-alex vasilev-alex merged commit 1ebd6a0 into cube-js:master Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature proposal pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query with many filter values ends up in 413 URL too large
5 participants