Check for duplicate source IDs

[ivovandongen]

ATM there is no check on source ID when adding it to the style. This leads to issues like #6612 (comment)

[ivovandongen]

@jfirebaugh Do you prefer throwing an exception or replacing the source?

cc @1ec5

[jfirebaugh]

I prefer throwing an exception, on the "fail fast and loud" principle. However, a C++ exception needs special handling and translation to SDK-appropriate error reporting at the SDK boundary.

[1ec5]

If mbgl throws an exception but you want the SDK to fail gracefully by replacing the source, you can catch the exception and remove and replace the source yourself.

The way sources are currently structured, the sources have no intrinsic order but each source has an intrinsic identifier. On iOS and macOS, that would support our current, set-like API of -addSource: and -removeSource:, and throwing an exception seems reasonable to me.

On the other hand, I’ve always wanted to think of a style as having sources keyed by their identifiers, which would be more consistent with the style specification’s sources property, which is an object rather than an array. For the iOS and macOS SDKs, that would imply a source API consisting of -setSource:forIdentifier: and -removeSourceForName:, where -setSource:forIdentifier: would silently replace a source if given an identifier twice. This would be consistent with our API for style images (-setImage:forName: and -removeImageForName:) and simplify some of the existing code in mbgl::style::Style. Would you be open to that approach, @jfirebaugh?

[jfirebaugh]

If -setSource:forIdentifier: allowed overwrites, it would suggest that you could change otherwise immutable properties of a source -- such as its tile URL or type -- by overwriting with a source where all the other properties are unchanged. And then a follow-on assumption would be that you could do that while there were existing layers using that source identifier, and those layers would automatically update. But you can't (#6612), and the changes required to support that would be complex.

Second, what are the use cases for overwriting an existing source, besides wanting to change properties that are intentionally immutable? I can't think of any. In most cases, you add a source, and then never touch it again. Duplicating a source identifier is likely an error.

[1ec5]

If -setSource:forIdentifier: allowed overwrites, it would suggest that you could change otherwise immutable properties of a source -- such as its tile URL or type -- by overwriting with a source where all the other properties are unchanged.

Something like this wouldn’t be possible, because url is read-only:

let source = style.sourceWithIdentifier("foo") as? MGLVectorSource
source.url = URL(string: "mapbox://something.else")
style.setSource(source, forIdentifier: "foo")

On the other hand, MGLGeoJSON does have a writeable features property, but that means no call to setSource(_:forIdentifier:) would be necessary.