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

Add push parameter checking and query installation #198

Merged

Conversation

wangmengyan95
Copy link
Contributor

This is the first step to implementing the push system in parse-server. In this PR, I add

  1. Parameter checking
  2. Run the installation query
    Since it is not finished yet, the api will still return error.

@wangmengyan95 wangmengyan95 force-pushed the wangmengyan.add_push_api_checking_and_run_queries branch 2 times, most recently from e4e44a6 to 8481e0b Compare February 3, 2016 08:19
@wangmengyan95
Copy link
Contributor Author

cc @bnham @fantasist for review

@nlutsenko
Copy link
Contributor

Please rebase after #196 is merged.

@wangmengyan95
Copy link
Contributor Author

@nlutsenko sure, will do that.

if (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a good way to expose internal methods for testing purpose? I follow the same way we do in JS and React repo, but I am not so confident about this. @peterdotjs @hallucinogen

Copy link
Contributor

Choose a reason for hiding this comment

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

We have used TESTING=1 which loads testing-routes, you could re-use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gfosco , haha, then I know why we have TESTING=1. But it seems we have a bug here. I remember all parameters in process.env is string so process.env.TESTING == 1 will always return false. Could you double check that?
If TESTING = 1 is just for test purpose, probably using process.env.NODE_ENV is a better idea. It is more common use than setting random variable and we can get rid of the integer/string stuff. I will send a PR if you do not mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that sounds fine to replace TESTING with NODE_ENV. Pretty sure JS coerces "1" == 1 just fine, it's "1" === 1 that would fail.

Choose a reason for hiding this comment

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

Commenting the original question: yes the check looks fine. Just make sure it's consistent across the code. In JS SDK we achieve this by caching the value in some JSON config https://github.com/ParsePlatform/Parse-SDK-JS/blob/master/src/CoreManager.js#L111

@wangmengyan95 wangmengyan95 force-pushed the wangmengyan.add_push_api_checking_and_run_queries branch 4 times, most recently from 9075fe2 to 65b19cd Compare February 3, 2016 18:20
@gfosco gfosco assigned wangmengyan95 and unassigned gfosco Feb 3, 2016
@wangmengyan95 wangmengyan95 assigned bnham and unassigned wangmengyan95 Feb 3, 2016
@wangmengyan95 wangmengyan95 force-pushed the wangmengyan.add_push_api_checking_and_run_queries branch from 65b19cd to fe4c1cc Compare February 4, 2016 08:31
if (!hasExpirationTime) {
return;
}
expirationTime = moment(new Date(body['expiration_time']));
Copy link

Choose a reason for hiding this comment

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

In the API, we allow expiration_time to be either a number or a string. If it's a number, then it's the number of seconds since the unix epoch. If it's a string, it's an ISO8601 string.

So this needs to be something like:

expirationTimeParam = body['expiration_time'];
if (typeof expirationTimeParam == 'number') {
  // date constructor wants ms since epoch, not seconds
  expirationTime = new Date(expirationTimeParam * 1000);
} else if (typeof expirationTimeParam == 'string') {
  expirationTime = new Date(expirationTimeParam);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@wangmengyan95 wangmengyan95 force-pushed the wangmengyan.add_push_api_checking_and_run_queries branch from fe4c1cc to 096c172 Compare February 5, 2016 07:40
@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

@wangmengyan95 wangmengyan95 force-pushed the wangmengyan.add_push_api_checking_and_run_queries branch from 096c172 to a683bef Compare February 5, 2016 07:59
@facebook-github-bot
Copy link

@wangmengyan95 updated the pull request.

body['expiration_time'] + ' is not valid time.');
}
// Check expirationTime is valid or not, if it is not valid, expirationTime is NaN
if (!isFinite(expirationTime)) {
Copy link

Choose a reason for hiding this comment

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

This seems like it might be some left over code from before? expirationTime is a Date, doesn't seem like it makes sense to call isFinite on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, isFinte can be used to check Date object directly, since it will auto transfer Date to number. If Date is invalid, it will return false, otherwise it will return true.

@bnham
Copy link

bnham commented Feb 5, 2016

LGTM, take a look at that isFinite call

wangmengyan95 added a commit that referenced this pull request Feb 5, 2016
…hecking_and_run_queries

Add push parameter checking and query installation
@wangmengyan95 wangmengyan95 merged commit 423c683 into master Feb 5, 2016
@wangmengyan95 wangmengyan95 deleted the wangmengyan.add_push_api_checking_and_run_queries branch February 5, 2016 21:29
montymxb pushed a commit to montymxb/parse-server that referenced this pull request Feb 14, 2016
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.

6 participants