Skip to content

Added public initializer to the ParseError struct #106

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

Closed
wants to merge 1 commit into from

Conversation

Vortec4800
Copy link

This is a very minor PR. Automatically generated initializers are only ever internal protection level, meaning they can't be used outside of the SDK. If a ParseError needs to be created from outside the library, like in a customer auth provider, a manual initializer must be provided.

This PR provides this initializer, which should match the generated initlizer. Code within the library should not need to change.

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #106 (5da8154) into main (f3f2244) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5da8154 differs from pull request most recent head 3bbe3a3. Consider uploading reports for the commit 3bbe3a3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
- Coverage   81.02%   81.02%   -0.01%     
==========================================
  Files          65       65              
  Lines        5477     5480       +3     
==========================================
+ Hits         4438     4440       +2     
- Misses       1039     1040       +1     
Impacted Files Coverage Δ
Sources/ParseSwift/Types/ParseError.swift 84.61% <100.00%> (+4.61%) ⬆️
Sources/ParseSwift/Storage/ParseFileManager.swift 89.39% <0.00%> (-0.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3f2244...3bbe3a3. Read the comment docs.

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 29, 2021

This is a very minor PR. Automatically generated initializers are only ever internal protection level, meaning they can't be used outside of the SDK. If a ParseError needs to be created from outside the library, like in a customer auth provider, a manual initializer must be provided.

Thanks for your PR... This was done on purpose as errors generated from the server and client are considered Parse Errors. Other errors are left to the developer. You should be able to create custom errors outside of Parse Errors within your app or SDK.

@Vortec4800
Copy link
Author

Hm, well that would mean that the app can't fulfil the ParseAuthentication protocol, as the required functions in the protocol require ParseErrors to be created, and there's no other way to create a ParseError. This means the handler would never be able to fail, even if it did fail. You also can't create a "subclass" of ParseError because it's a struct, so no inheritance.

I think we either need to make it so you can create ParseErrors outside of the SDK, or change the authentication handler response to accept generic Swift Errors instead of needing a ParseError.

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 29, 2021

Hm, well that would mean that the app can't fulfil the ParseAuthentication protocol, as the required functions in the protocol require ParseErrors to be created, and there's no other way to create a ParseError. This means the handler would never be able to fail, even if it did fail. You also can't create a "subclass" of ParseError because it's a struct, so no inheritance.

I currently don't see why this would be needed. Parse Swift doesn't use classes, so there's no subclasses available or needed for any of the Authentication methods. It's all protocol and struct based. I think once you open a PR and start implementing it I will be able to show how to populate the error as it won't be direct rip from the objective-c implementation which seems like what you are describing.

When you crate the PR, you are adding the authentication method to the SDK, meaning it has access to ParseError and can use the internal member wise initializer, you don't need the member wise initializer to be public inside the SDK. Outside the SDK, you don't create ParseErrors as those are not ParseErrors, they are developer specific errors. I recommend looking at ParseAuthentication and ParseLDAP implementation first. Are you saying you need to do something completely different than what's already done? My brief look at restoreAuthentication seems like it's a straight-forward addition.

Inside ParseLDAP, an error is created using the member wise initializer:

func link(authData: [String: String],
options: API.Options = [],
callbackQueue: DispatchQueue = .main,
completion: @escaping (Result<AuthenticatedUser, ParseError>) -> Void) {
guard AuthenticationKeys.id.verifyMandatoryKeys(authData: authData) else {
let error = ParseError(code: .unknownError,
message: "Should have authData in consisting of keys \"id\" and \"password\".")
callbackQueue.async {
completion(.failure(error))
}
return
}

You can do the something for restoreAuthentication

@Vortec4800
Copy link
Author

Sorry, this is a separate issue from my restoreAuthentication delegate issue I raised - though I ultimately need them both for my task.

This is a custom auth provider that will live in my app code, not in the library, so there won't be a PR. This is to authenticate against an internal system we use at our company, so it wouldn't be appropriate to add to the library as it's hyper-specific to our use case. In the provider, one of the protocol methods is:

	func login(authData: [String: String],
			   options: API.Options = [],
			   callbackQueue: DispatchQueue = .main,
			   completion: @escaping (Result<AuthenticatedUser, ParseError>) -> Void) {
		guard AuthenticationKeys.id.verifyMandatoryKeys(authData: authData) else {
			callbackQueue.async {
				completion(.failure(.init(code: .unknownError, message: "authData is missing required values.")))
			}
			return
		}
		AuthenticatedUser.login(Self.__type, authData: authData, options: options, callbackQueue: callbackQueue, completion: completion)
	}

However, that won't compile because I'm trying to create a ParseError for the completion handler in the case that the authData provided is incorrect. As the failure state in the handler requires a ParseError, I don't see another way I can mark this as a failure.

Perhaps a custom ParseError case could be created for a non-parse error, and a public initializer would only create ParseErrors in that specific case with a custom error message?

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 29, 2021

Perhaps a custom ParseError case could be created for a non-parse error, and a public initializer would only create ParseErrors in that specific case with a custom error message?

If I understand correctly, you are trying to conform to ParseAuthentication by creating creating a custom authentication method that will live outside the SDK, but the current ParseAuthentication protocol implementation won't allow this because it requires you to create ParseErrors?

@Vortec4800
Copy link
Author

That sounds exactly correct.

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 29, 2021

I will have to think about this some, but my initial thoughts are:

  • (this option isn't the best option) You can implement your own version of ParseAuthentication and pass custom errors
  • (probably better) Submit a PR allowing ParseAuthentication to return Error instead of requiring ParseError. In this case, the SDK implementation methods can return ParseError (Since they are Error underneath) when needed and private custom implementations can return their own versions of Error.

@Vortec4800
Copy link
Author

Out of the two, I would definitely prefer the second option. I think the first would be a pretty high barrier for entry if other people are looking to do the same thing.

The only downside I see of the second option is that you'll need to do some extra work elsewhere in the SDK to verify that the errors you get back are indeed ParseErrors, now that the protocol doesn't explicitly require them to be so, and then it would be prudent to handle both scenarios - ParseErrors and generic Errors.

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 29, 2021

The only downside I see of the second option is that you'll need to do some extra work elsewhere in the SDK to verify that the errors you get back are indeed ParseErrors, now that the protocol doesn't explicitly require them to be so, and then it would be prudent to handle both scenarios - ParseErrors and generic Errors.

You might get lucky with this as the SDK first attempts to decode the error from the server as a ParseError, if its not a ParseError, it turns the error into one. So it's possible it "may" not be a lot of work, but I'm guessing...

@Vortec4800
Copy link
Author

Okay, let me finish my current work with the Easy Way (just using my init from this PR) and once I've verified that's working, I'll try swapping to a generic Error and make the changes for that, then I'll send in a separate PR for that portion and abandon this one.

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 30, 2021

Thinking about this some more the internal authentication types lose some identity with ParseError by implementing the second method. This means that other devs can't create switch/if statements off of ParseError.code or be able to access message.

If you are doing a custom conformance it might be best for you to just encode/decode ParseError to/from your custom created error (you can accomplish this using Swifts standard JSONEncoder/Decoder). That will allow you to add more error properties if you want and maintain the original ParseError. I don't think making the initializer public is the right solution either as developers shouldn't create errors and mask them as Parse Errors as that will lead people to believe they are real Parse Errors and encourage them to submit issues to this repo while they should be submitted to the specific repo that's using ParseSwift.

I suggest you post your feedback before starting the implementation on the PR so you spend your time on the right solution.

@Vortec4800
Copy link
Author

I see what you're saying but it still feels like quite a workaround. Plus ultimately we'd just be creating our own error, then serializing it, and using that payload to create a ParseError anyway and returning that. Just feels like a bunch of extra steps with the same end result: a ParseError that may be misleading as it technically originated outside of the Parse SDK.

Let me think on it some more.

@cbaker6
Copy link
Contributor

cbaker6 commented Mar 30, 2021

Plus ultimately we'd just be creating our own error, then serializing it, and using that payload to create a ParseError anyway and returning that. Just feels like a bunch of extra steps with the same end result:

True, but making ParseAuthentication use Error means that developers of the provided auth types can’t use ParseError directly (which will probably be most devs). The smaller amount of devs who need custom/private auth types need to take the extra steps (which isn't really a lot, just create 1/2 methods that encode/decode the ParseError, then create your own error) and know that they are circumventing PareError to create their own error.

If you are writing many custom auths, you can probably create a protocol that conforms to ParseAuthentication that has default implementations of the error encoding/decoding to handle this for you.

@cbaker6 cbaker6 marked this pull request as draft April 2, 2021 23:18
@cbaker6
Copy link
Contributor

cbaker6 commented May 18, 2021

Closing this due to inactivity. Feel free to open if more questions.

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