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 get and set server #1464

Merged
merged 3 commits into from
Nov 25, 2019

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Oct 30, 2019

Added the possibility to set and get the current server URL without having to destroy and re-initialize the client. Example use case: domain failover handling.

Analogous to parse-community/Parse-SDK-Android#985

@mtrezza
Copy link
Member Author

mtrezza commented Oct 30, 2019

The failing tests do not seem to be concerning this PR.

Copy link

@mrmarcsmith mrmarcsmith left a comment

Choose a reason for hiding this comment

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

I like this idea! I haven't dug deep into the nuts and "Bolts" yet. (yes, that was a pun) is there any danger of messing up a request or connection by changing the server URL on the fly? should there be logic to invalidate the current connection to avoid weird TLS bugs?

@mtrezza
Copy link
Member Author

mtrezza commented Oct 30, 2019

@mrmarcsmith No issues found so far.

Is it merely the server URL that is changed. A ParseRequest needs the Parse client for callback after receiving the response, the client instance stays alive when changing the URL.

Regarding TSL, I have tested switching over to a different URL with the same certificate (multi host certificate), and it worked. I haven’t tested switching to a URL with a different certificate, but I don’t see why this would be an issue. I’d see this happening on a different layer that should be unaffected by the URL change.

Issues were only observed if the client is destroyed and re-initialized while tasks are waiting for server response, which is not part of this PR.

Sent with GitHawk

Copy link
Member

@drdaz drdaz left a comment

Choose a reason for hiding this comment

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

Yeah I like it too. And I don't notice anything dangerous in the code.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 1, 2019

don't merge, I may have found an issue, investigating...

@mtrezza
Copy link
Member Author

mtrezza commented Nov 1, 2019

Turns out the log below was because I had a domain blocked on the firewall for testing the automatic host failover. "SSL error has occurred" was misleading me, when all outgoing packages were simply blocked.

Error Domain=NSURLErrorDomain Code=-1200 "An SSL error has occurred and a secure connection to the server cannot be made." UserInfo={_kCFStreamErrorCodeKey=-9816, NSLocalizedRecoverySuggestion=Would you like to connect to the server anyway?, NSUnderlyingError=0x600002dc6c40 {Error Domain=kCFErrorDomainCFNetwork Code=-1200 "(null)" UserInfo={_kCFStreamPropertySSLClientCertificateState=0, _kCFNetworkCFStreamSSLErrorOriginalValue=-9816, _kCFStreamErrorDomainKey=3, _kCFStreamErrorCodeKey=-9816}}, NSLocalizedDescription=An SSL error has occurred and a secure connection to the server cannot be made., NSErrorFailingURLKey=https://example.com.com/parse/health, NSErrorFailingURLStringKey=https://example.com.com/parse/health, _kCFStreamErrorDomainKey=3}


However, before merge, I will do some further testing, just to be sure.

Copy link

@mrmarcsmith mrmarcsmith left a comment

Choose a reason for hiding this comment

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

I’m comfortable with it

@mtrezza
Copy link
Member Author

mtrezza commented Nov 1, 2019

did some more testing, no issues found, good to merge

@mtrezza
Copy link
Member Author

mtrezza commented Nov 13, 2019

@noobs2ninjas any time estimate when you can review this?

@TomWFox
Copy link
Contributor

TomWFox commented Nov 13, 2019

I’m happy with this, we can always merge and then revert if needed, @noobs2ninjas hasn't been active for a couple weeks.

@noobs2ninjas
Copy link
Member

Sorry for the delay guys. New job. I'm ok with this one. Although, I must say that this borderlines niche use code. So, let's try to keep that in mind going forward. Not a whole lot of people would find a use for this and introducing it could cause confusion. For example. Someone new to Parse could see this and easily be fooled into thinking that they can initialize parse without setting the configuration. So, let's make sure this gets added and explained properly in the docs.

either way this should be good. I know bolts well and this shouldn't mess anything up there as long as there aren't any current parse requests running when this gets called.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 22, 2019

@noobs2ninjas

Although, I must say that this borderlines niche use code. So, let's try to keep that in mind going forward.

Can you please explain what you conclude by that? What should be kept in mind regarding “niche code” going forward?

Not a whole lot of people would find a use for this

What is the basis for your conclusion?

So, let's make sure this gets added and explained properly in the docs.

API docs have been added. Is there anything specific you would like to add?

this shouldn't mess anything up there as long as there aren't any current parse requests running when this gets called.

Can you reference in Bolts where this does mess up? I tested this thoroughly and changing the URL string even while requests are running did not seem to mess anything up.

@noobs2ninjas
Copy link
Member

@mtrezza

Can you please explain what you conclude by that? What should be kept in mind regarding “niche code” going forward?

By niche code. I really mean framework functionality that has niche use aka "Not a whole lot of people would find a use for this"

What is the basis for your conclusion?
I have used parse since 2012 for over 75+ projects now and have never come across a need to have multiple Parse databases. Additionally, we certainly haven't had many(if any) requests for this.

The real question is what is the basis for why you think Parse needs this?

Can you reference in Bolts where this does mess up? I tested this thoroughly and changing the URL string even while requests are running did not seem to mess anything up.

I'm not saying its very likely at all but I'm 99% sure its possible. It's more on Parse side than on Bolts. Consider potential real world network availability in rural areas of some countries included the united states. Having several outstanding saveEventually() calls when there is no network availability and then changing the server url prior to regaining coverage.

In fact if we really want to go down the road where Parse could get messed up by this. Let's consider applications that subclass PFObject, use PFUser or PFSession, or user local data storage and pin objects. In fact I would go so far as to say that if we really wanted to implement this properly then we should be automatically clearing all cache, session, and user data when switching urls. I'm sure you know enough about those things to steer clear of those issue. However, if I'm being frank, this repo was specifically designed so that developers don't have to care about those things.

I'm considering that we should consider not approving this merge being that none of those issues have been addressed with this push. Parse was created for developers who either dont consider or dont want to consider everything that happens in the background. If you look at the API on the whole pretty much every function is pretty self explanatory and has pretty much 0 caveats. This function definitely comes with a TON of caveats to consider before a developer should incorporate this into their code.

Copy link
Member

@noobs2ninjas noobs2ninjas left a comment

Choose a reason for hiding this comment

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

We should, at the minimum, have a devPrint advising that using this function on applications that rely on User, Session, or Local Data Storage, could have unforeseen consequences.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 25, 2019

@noobs2ninjas

have never come across a need to have multiple Parse databases.

That is not the topic of this PR, it's about changing the URL.

Additionally, we certainly haven't had many(if any) requests for this

parse-community/Parse-SDK-Android#521
#1390
#1209
...and more on StackOverflow
In addition, this feature already exists in the Android SDK.

We should, at the minimum, have a devPrint advising that using this function on applications that rely on User, Session, or Local Data Storage, could have unforeseen consequences.

Can you point to the specific code, so we can fix and prevent any unforeseen consequences?
I see 2 scenarios:

  1. The DB does not change: I don't see any caching issues. On the contrary, I wouldn't want to clear the cache, because I want the user to stay logged in.
  2. The DB changes: The session would be invalid and the current PFUser invalidated. I think adding a comment of caution for this scenario can't be wrong. I'll add that to the PR.

The real question is what is the basis for why you think Parse needs this?

@flovilmart's comment: "I'll gladly review a PR that would bring that feature in."

Imagine the parse server connection of one of your 75+ Parse projects suddenly fails, it would take days to push an app update to the users. A single endpoint is a significant business risk. I think it would be beneficial for the Parse community to have this feature to implement server connection failover.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 25, 2019

@noobs2ninjas how about this:

Screen Shot 2019-11-25 at 03 37 19

@noobs2ninjas
Copy link
Member

@mtrezza

Man. My bad. Somehow I completely missed the last sentence of your description "Example use case: domain failover handling." I read the pull request name and saw the function name and assumed you where just trying to change parse databases. Which, under those circumstances, would cause User, Session, and Local Data to become unusable as no objectId's or session/auth tokens would match. Sorry dude.

I like the warning. I think it works. Function naming alone does leave a bit open to interpretation. Best we can do is try and protect the user by documenting in the framework and on the guide.

Understanding now what you are trying to do with it I'm totally behind this. Although 3 issues being created over 2 repos in the last 5 years isn't a lot, I think this is one of those cases where most developers, myself included, just never considered it an option.

In fact, if you dont mind, whenever you get time, I'd like to ask you to consider taking this one step further and think about possibly extending this into the ParseClientConfiguration and adding code that automatically can switch over to the failover url. Something like.

let parseConfig = ParseClientConfiguration {
    $0.applicationId = "parseAppId"
    $0.clientKey = "parseClientKey"
    $0.server = "parseServerUrlString"
    $0.failoverUrl = "parseServerFailoverUrl"
}
Parse.initialize(with: parseConfig)

I see you've been contributing all around Parse for quite a while now. Hopefully you understand I was just playing the devils advocate. If Parse wouldn't have made things so easy for me back when it came out I'm not sure I'd be a developer period much less doing iOS professionally. So, I just try to come at things from all angles to make sure Parse can still do for other people what it did me.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 25, 2019

@noobs2ninjas

adding code that automatically can switch over to the failover url

I don't see myself tackling this at the moment.

Parse was created for developers who either dont consider or dont want to consider everything that happens in the background. (...) So, I just try to come at things from all angles to make sure Parse can still do for other people what it did me.

This is off topic, but I disagree with your characterization of the Parse target audience. It's not 2011 anymore and Parse Server is not Parse.com. I assume that fletching developers who want an easy MBaaS will go for Firebase or a hosted Parse alternative with an abstract dashboard and no need for server and database management. The main reason why I see Parse Server as so important today is because it is a sophisticated Open Source alternative that does not lock one in on the arbitrary goodwill of a for-profit corporation. The key differentiator for Parse Server is not simplicity for the novice developer anymore, it has to be open source. As such we should very much appreciate any effort that comes with any reasonable PR offered to the Parse community and constructively refine it, rather than bureaucratically debating whether it's "niche code". That can only demotivate contributors and is detrimental to the community.

@noobs2ninjas
Copy link
Member

@mtrezza

I don't believe in just approving every single pull request that comes through for the sakes of making someone feel like they contributed. I believe that the main repo is for the masses and not for the individual and that forking exists for a reason. You may not agree with that mindset, but thats ok. Either way, I already said that I misunderstood your intentions with this and you have your approval. You can merge if you have the option or I can do it for you.

Please be advised that pushing new releases is being delayed. We are dealing with poorly maintained repos(Bolts) and the way Xcode 11 environment tries to convert iOS build targets to catalyst automatically. Give it a few weeks though and we'll resume a regular release schedule. Although your opinion of me may disagree, I do appreciate your contribution and your patients.

Thanks!

@mtrezza
Copy link
Member Author

mtrezza commented Nov 25, 2019

@noobs2ninjas That's fair. Yes, please go ahead merge :)

@noobs2ninjas noobs2ninjas merged commit 7be3ea3 into parse-community:master Nov 25, 2019
@noobs2ninjas
Copy link
Member

noobs2ninjas commented Nov 25, 2019

@mtrezza

Sorry, one last question. By "amended docs" you meant the frameworks internal documentation aka what fuzzy generates for our API doc site and not the "docs" repo where we maintain documentation and guides on http://parseplatform.org/ right? No big deal if you didn't. I just need to add a task over there and we'll get it added later.

@mtrezza
Copy link
Member Author

mtrezza commented Nov 25, 2019

@noobs2ninjas Correct, doc is only framework internal for now.

@noobs2ninjas
Copy link
Member

Perfect. Thanks again!

@mtrezza mtrezza deleted the add-get-and-set-server branch January 13, 2020 16:17
@mtrezza mtrezza restored the add-get-and-set-server branch January 13, 2020 16:18
@noobs2ninjas noobs2ninjas linked an issue Mar 10, 2020 that may be closed by this pull request
@TomWFox TomWFox added the type:feature New feature or improvement of existing feature label Apr 3, 2020
@AliZahr
Copy link

AliZahr commented Aug 10, 2020

I tried to set a new parse server url while inside the app, but there was no change what so ever.

Parse.server = @"https://serverNewUrl.com"; or [Parse setServer:@"https://serverNewUrl.com"];
NSLog(@"%@",Parse.server); -----> This prints the new server set
NSLog(@"%@",Parse.currentConfiguration.server); -----> This prints the old one set

How did you achieve the change ?

@mtrezza
Copy link
Member Author

mtrezza commented Aug 10, 2020

@AliZahr For help with Parse Server, here are some resources you can try:

  • For code-level questions we recommend Stack Overflow using the parse-platform tag.
  • For questions that are not appropriate for the above mentioned sites we recommend our community forum.

@mtrezza mtrezza deleted the add-get-and-set-server branch November 19, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swift OSX - Change parse server URL
6 participants