Skip to content

fix: Parse.server doesn't return server url#1821

Open
dplewis wants to merge 4 commits into
parse-community:masterfrom
dplewis:get-server
Open

fix: Parse.server doesn't return server url#1821
dplewis wants to merge 4 commits into
parse-community:masterfrom
dplewis:get-server

Conversation

@dplewis

@dplewis dplewis commented Oct 31, 2024

Copy link
Copy Markdown
Member

New Pull Request Checklist

Issue Description

ParseClientConfiguration *config is never used, when the current parse manager isn't set no url is returned.

Approach

  • Get server URL from proper configuration

@parse-github-assistant

parse-github-assistant Bot commented Oct 31, 2024

Copy link
Copy Markdown

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

@dplewis
dplewis requested a review from a team October 31, 2024 16:46
@dplewis dplewis changed the title fix: Parse.server doesn't return server url fix: Parse.server doesn't return server url Oct 31, 2024
@mtrezza

mtrezza commented Oct 31, 2024

Copy link
Copy Markdown
Member

The SDK has a feature where the server URL can be changed on the fly, after Parse SDK init. Could this have any impact on that feature? I'm not sure we have a test for this?

#729
#1464

@dplewis

dplewis commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

Doesn't have any impact on that feature

@mtrezza mtrezza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And could you add a test for this fix?

@codecov

codecov Bot commented Oct 31, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.68%. Comparing base (dd05d41) to head (dba42f3).
Report is 43 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1821       +/-   ##
===========================================
+ Coverage   64.24%   82.68%   +18.44%     
===========================================
  Files         201      282       +81     
  Lines       23233    30750     +7517     
===========================================
+ Hits        14926    25426    +10500     
+ Misses       8307     5324     -2983     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dplewis

dplewis commented Nov 1, 2024

Copy link
Copy Markdown
Member Author

@mtrezza I don't think setServer works. There are two or more configurations being managed. There are no tests for this but I wrote one.

- (void)testSetServerURL {
    ParseClientConfiguration *configuration = [ParseClientConfiguration configurationWithBlock:^(id<ParseMutableClientConfiguration> configuration) {
        configuration.applicationId = @"foo";
        configuration.clientKey = @"bar";
        configuration.server = @"http://localhost";
        configuration.localDatastoreEnabled = YES;
        configuration.networkRetryAttempts = 1337;
    }];

    XCTAssertEqualObjects(configuration.applicationId, @"foo");
    XCTAssertEqualObjects(configuration.clientKey, @"bar");
    XCTAssertTrue(configuration.localDatastoreEnabled);
    XCTAssertEqual(configuration.networkRetryAttempts, 1337);
    XCTAssertEqualObjects(configuration.server, @"http://localhost");

    NSLog(@"%@", Parse.server); // https://api.parse.com/1 ???

    [Parse setServer:@"http://example.org"]; // Error: You must set your configuration's `applicationId` before calling +[Parse initializeWithConfigurationAllowingReinitialize:]! (NSInternalInconsistencyException)
}

@mtrezza

mtrezza commented Nov 1, 2024

Copy link
Copy Markdown
Member

Parse.setServer should work, the client needs to be initialized. This is for setting the server on the fly, not for setting the config in preparation for SDK init. Thanks for adding a test though, I was actually referring to a test for the changed line in this PR, but if we have a test for setServer as well, even better.

@dplewis

dplewis commented Nov 1, 2024

Copy link
Copy Markdown
Member Author

@mtrezza Can you add a test case for setServer? I saw your last PR #1708 but no test cases was added. I may have missed something. Just copy my test and add it to ParseClientConfigurationTests with #import "Parse_Private.h" at the top

@mtrezza

mtrezza commented Nov 1, 2024

Copy link
Copy Markdown
Member

Unfortunately I'm lacking the toolchain at the moment.

@dplewis

dplewis commented Jan 28, 2025

Copy link
Copy Markdown
Member Author

@mtrezza do you have the toolchain now to write the test?

@mtrezza

mtrezza commented Jan 28, 2025

Copy link
Copy Markdown
Member

Yes, but no resources atm

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.

2 participants