From abe085614fbd469ad1ba69dd195c45185f43b0c5 Mon Sep 17 00:00:00 2001 From: hideya kawahara Date: Fri, 3 Jul 2015 20:04:38 +0900 Subject: [PATCH] Fix the URL generation code so that it won't attach bogus parameters. Before the fix, for example, [LBFileTests testUpload] generated a path like: "/containers/container1/files/f1.txt?container=container1&name=f1.txt" whereas it should have just been "/containers/container1/files/f1.txt". To fix the issue [SLRESTContract urlForMethod:parameters:] and [SLRESTContract urlWithPattern:parameters:] now take NSMutableDictionary as the parameters argument so that the key-value pairs which are consumed when generating the URL can be removed. --- SLRemoting/SLRESTAdapter.m | 5 +++-- SLRemoting/SLRESTContract.h | 13 +++++++++---- SLRemoting/SLRESTContract.m | 17 +++++++++++------ SLRemotingTests/SLRESTContractTests.m | 16 +++++++++++++--- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/SLRemoting/SLRESTAdapter.m b/SLRemoting/SLRESTAdapter.m index 3aacb97..acfce05 100644 --- a/SLRemoting/SLRESTAdapter.m +++ b/SLRemoting/SLRESTAdapter.m @@ -82,13 +82,14 @@ - (void)invokeStaticMethod:(NSString *)method NSAssert(self.contract, @"Invalid contract."); + NSMutableDictionary *mutableParams = [NSMutableDictionary dictionaryWithDictionary:parameters]; NSString *verb = [self.contract verbForMethod:method]; - NSString *path = [self.contract urlForMethod:method parameters:parameters]; + NSString *path = [self.contract urlForMethod:method parameters:mutableParams]; BOOL multipart = [self.contract multipartForMethod:method]; [self requestWithPath:path verb:verb - parameters:parameters + parameters:mutableParams multipart:multipart outputStream:outputStream success:success diff --git a/SLRemoting/SLRESTContract.h b/SLRemoting/SLRESTContract.h index 8e73313..60064d7 100644 --- a/SLRemoting/SLRESTContract.h +++ b/SLRemoting/SLRESTContract.h @@ -120,13 +120,14 @@ extern NSString *SLRESTContractDefaultVerb; /** * Resolves a specific method, replacing pattern fragments with the optional * `parameters` as appropriate. + * Also removes consumed key-value pairs from the mutable `parameters`. * * @param method The method to resolve. * @param parameters Pattern parameters. Can be `nil`. * @return The complete, resolved URL. */ - (NSString *)urlForMethod:(NSString *)method - parameters:(NSDictionary *)parameters; + parameters:(NSMutableDictionary *)parameters; /** * Returns the HTTP verb for the given method string. @@ -162,15 +163,19 @@ extern NSString *SLRESTContractDefaultVerb; - (NSString *)patternForMethod:(NSString *)method; /** - * Returns a rendered URL pattern using the parameters provided. For example, - * `@"/widgets/:id"` + `@{ @"id": "57", @"price": @"42.00" }` begets + * Returns a rendered URL pattern using the parameters provided. + * Also removes consumed key-value pairs from the mutable `parameters`. + * For example, + * `@"/widgets/:id"` + `@{ @"id": @"57", @"price": @"42.00" }` begets * `@"/widgets/57"`. + * And `parameter` becomes `@{ @"price": @"42.00" }` * * @param pattern The pattern to render. * @param parameters Values to render with. + * This is mutable and consumed key-value pairs get removed. * @return The rendered URL. */ - (NSString *)urlWithPattern:(NSString *)pattern - parameters:(NSDictionary *)parameters; + parameters:(NSMutableDictionary *)parameters; @end diff --git a/SLRemoting/SLRESTContract.m b/SLRemoting/SLRESTContract.m index 3c18f18..e17d35f 100644 --- a/SLRemoting/SLRESTContract.m +++ b/SLRemoting/SLRESTContract.m @@ -131,7 +131,7 @@ - (void)addItemsFromContract:(SLRESTContract *)contract { } - (NSString *)urlForMethod:(NSString *)method - parameters:(NSDictionary *)parameters { + parameters:(NSMutableDictionary *)parameters { NSParameterAssert(method); NSString *pattern = [self patternForMethod:method]; @@ -173,18 +173,23 @@ - (NSString *)patternForMethod:(NSString *)method { } - (NSString *)urlWithPattern:(NSString *)pattern - parameters:(NSDictionary *)parameters { + parameters:(NSMutableDictionary *)parameters { NSParameterAssert(pattern); if (!parameters) { return pattern; } - NSString __block *url = pattern; + NSString *url = pattern; - [parameters enumerateKeysAndObjectsUsingBlock:^(id key, id obj, BOOL *stop) { - url = [url stringByReplacingOccurrencesOfString:[NSString stringWithFormat:@":%@", key] withString:[NSString stringWithFormat:@"%@", obj]]; - }]; + for (NSString *key in parameters.allKeys) { // create a copy of allKeys to mutate parameters + NSString *keyPattern = [NSString stringWithFormat:@":%@", key]; + if ([url rangeOfString:keyPattern].location == NSNotFound) continue; + + NSString *valueStr = [NSString stringWithFormat:@"%@", parameters[key]]; + url = [url stringByReplacingOccurrencesOfString:keyPattern withString:valueStr]; + [parameters removeObjectForKey:key]; + } return url; } diff --git a/SLRemotingTests/SLRESTContractTests.m b/SLRemotingTests/SLRESTContractTests.m index 762b025..2825705 100644 --- a/SLRemotingTests/SLRESTContractTests.m +++ b/SLRemotingTests/SLRESTContractTests.m @@ -16,7 +16,6 @@ @interface SLRESTContractTests() { SLRESTAdapter *adapter; - SLRESTContract *contract; SLRepository *TestClass; } @@ -42,6 +41,17 @@ - (void)tearDown { [super tearDown]; } +- (void)testUrlWithPattern { + SLRESTContract *contract = [SLRESTContract contract]; + NSMutableDictionary *parameters = [@{ @"id": @"57", @"price": @"42.00" } mutableCopy]; + + NSString *url = [contract urlWithPattern:@"/widgets/:id" parameters:parameters]; + + STAssertEqualObjects(url, @"/widgets/57", @"Invalid URL"); + STAssertEqualObjects(parameters, [@{ @"price": @"42.00" } mutableCopy], @"Invalid parameters"); + NSLog(@"\n***, %@, %@", url, parameters); +} + - (void)testAddItemsFromContract { SLRESTContract *parent = [SLRESTContract contract]; SLRESTContract *child = [SLRESTContract contract]; @@ -51,9 +61,9 @@ - (void)testAddItemsFromContract { [child addItem:[SLRESTContractItem itemWithPattern:@"/new/route" verb:@"POST"] forMethod:@"new.route"]; [parent addItemsFromContract:child]; - STAssertTrue([[parent urlForMethod:@"test.route" parameters:@{}] isEqualToString:@"/test/route"], @"Wrong URL."); + STAssertTrue([[parent urlForMethod:@"test.route" parameters:nil] isEqualToString:@"/test/route"], @"Wrong URL."); STAssertTrue([[parent verbForMethod:@"test.route"] isEqualToString:@"GET"], @"Wrong verb."); - STAssertTrue([[parent urlForMethod:@"new.route" parameters:@{}] isEqualToString:@"/new/route"], @"Wrong URL."); + STAssertTrue([[parent urlForMethod:@"new.route" parameters:nil] isEqualToString:@"/new/route"], @"Wrong URL."); STAssertTrue([[parent verbForMethod:@"new.route"] isEqualToString:@"POST"], @"Wrong verb."); }