Parse-SDK-iOS-OSX icon indicating copy to clipboard operation
Parse-SDK-iOS-OSX copied to clipboard

fix: `Parse.server` doesn't return server url

Open dplewis opened this issue 1 year ago • 10 comments

New Pull Request Checklist

  • [ ] I am not disclosing a vulnerability.
  • [ ] I am creating this PR in reference to an issue.

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

dplewis avatar Oct 31 '24 16:10 dplewis

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.

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?

https://github.com/parse-community/Parse-SDK-iOS-OSX/pull/729 https://github.com/parse-community/Parse-SDK-iOS-OSX/pull/1464

mtrezza avatar Oct 31 '24 19:10 mtrezza

Doesn't have any impact on that feature

dplewis avatar Oct 31 '24 19:10 dplewis

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 31 '24 22:10 codecov[bot]

@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)
}

dplewis avatar Nov 01 '24 01:11 dplewis

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.

mtrezza avatar Nov 01 '24 01:11 mtrezza

@mtrezza Can you add a test case for setServer? I saw your last PR https://github.com/parse-community/Parse-SDK-iOS-OSX/pull/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

dplewis avatar Nov 01 '24 01:11 dplewis

Unfortunately I'm lacking the toolchain at the moment.

mtrezza avatar Nov 01 '24 01:11 mtrezza

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

dplewis avatar Jan 28 '25 15:01 dplewis

Yes, but no resources atm

mtrezza avatar Jan 28 '25 17:01 mtrezza