Skip to content
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

Multiplatform OSM API client #5410

Closed
westnordost opened this issue Dec 20, 2023 · 15 comments · Fixed by #5686
Closed

Multiplatform OSM API client #5410

westnordost opened this issue Dec 20, 2023 · 15 comments · Fixed by #5686
Assignees
Labels
iOS necessary for iOS port

Comments

@westnordost
Copy link
Member

Requires #5408 to be done first.

The OSM API is a HTTP REST API that communicates chiefly in XML. This must be ported to Kotlin multiplatform.

Currently, the Java library de.westnordost.osmapi is used for any communication with the OSM API 0.6, i.e. uploading map data, notes and gpx tracks, downloading map data, notes, user data, etc.

The dependency to osmapi must be replaced with an implementation based on

  • ideally the http client from Multiplatform HTTP Client (use Ktor Client) #5408 and

  • a multiplatform XML parser / writer, which is stable, well-tested and ideally (semi-)official / well-maintained and ideally but not necessarily compatible with kotlinx-serialization. This is also important for other XML parsing in the app, i.e. Add GPX import functionality #5369

  • using kotlinx-datetime to parse timestamps (already a dependency)

See interfaces TracksApi, MapDataApi and NotesApi

XML Parser / Writer requirements

  • multiplatform support for at least Java/Android and iOS

  • stable, well-tested and ideally (semi-)official / well-maintained

xmlutil

There is currently one xml serializer compatible with kotlinx-serialization: io.github.pdvrieze.xmlutil. It is also mentioned in the Ktor docs, i.e. is the XML serializer/deserializer used in Ktor.

I didn't find any other multiplatform XML parsers/writers.

@westnordost westnordost added blocked blocked by another issue iOS necessary for iOS port labels Dec 20, 2023
@westnordost westnordost moved this to Blocked in iOS Port Dec 20, 2023
@susrisha
Copy link

I would like to pick this up. I will ping back after some basic implementation.

@westnordost
Copy link
Member Author

This is blocked by #5408, i.e. that should be done first.

@westnordost
Copy link
Member Author

westnordost commented Jan 30, 2024

Because, the thinking is the following:

  1. Both the access to the OSM API and the other APIs we are accessing should be done with the same library in order to both reduce the number of dependencies and to reduce the amount of brain-work to familiarize oneself with a library

  2. It would be possible to write a full kotlin multiplatform or iOS replacement of the Java library de.westnordost.osmapi instead, in a separate repository. de.westnordost.osmapi covers the whole of the OSM API. But re-implementing that library would be considerably more work than just implementing the (few) OSM API calls we actually use, plus, more continuous maintenance work, because the OSM API has been extended over the years, so the expectation is that if a kotlin multplatform or even an iOS version of that library is created, it will be maintained equally well. (And if it is not maintained, then maybe we don't even want to use it in STreetComplete, because we have had bad experiences with using libraries that were badly maintained or were stopped being maintained)

@susrisha
Copy link

My intention is to build an equivalent of de.westnordost.osmapi so that it can be used in multiplatform. Using Ktor client seems a viable option and that can be extended for other API calls within the project. I understand that the maintenance will take its toll but I beleive Ktor is here to stay. Only caveat we found was with the deserialization of xml. Though ktor provides a library, its only JVM compatible. With io.github.pdvrieze.xmlutil, I think it makes a better sense. Let me know if we have decided on HTTP Client

@westnordost
Copy link
Member Author

Cool! Indeed. Yes, xmlutil should work. The only http client that is multiplatform also for native is Ktor Client, as far as I know, so the decision is to use Ktor. OkHttp 5 is only multiplatform jvm+js.

@westnordost westnordost moved this from Blocked to Todo in iOS Port Jan 31, 2024
@susrisha
Copy link

susrisha commented Feb 4, 2024

@westnordost we started off simple in an independent repository. I gave you read access. We can move it to any other repo once its in a decent shape

@westnordost
Copy link
Member Author

westnordost commented Feb 11, 2024

I can have a look at it when it is public, if you like, though bear in mind that I have zero experience with Ktor Client so far. The OSM API on the other hand I know inside-out.

@susrisha
Copy link

We have come to a decent shape on the api. We may need help in how to publish it for using in other projects. This may be a bit tricky.

@westnordost
Copy link
Member Author

Not sure what you mean.

Java/Kotlin FOSS libraries are generally published on maven central via sonatype. If you are not sure what to write into the build gradle, I recently published a multiplatform library too, check https://github.com/westnordost/osm-opening-hours/blob/master/build.gradle.kts
If I remember correctly, you need an account at sonatype to be able to publish artifacts, you'll need to read a guide for that because it is too long ago that I did that.

If it is regarding the license: Any open source license would do, permissive or LGPL, even GPLv3 would be fine.

@susrisha
Copy link

The code is almost ready to be made public. @westnordost need advice on the package name though. right now its com.example.osmapi . What do you suggest?

@westnordost
Copy link
Member Author

westnordost commented Feb 16, 2024 via email

@susrisha
Copy link

Its hosted under vindago org. So, we will name it com.vindago.osmapi Thanks for the suggestion

@westnordost
Copy link
Member Author

westnordost commented Feb 16, 2024 via email

@susrisha
Copy link

susrisha commented Feb 22, 2024

@westnordost here is the public link to the API library. its not complete yet but its a start. https://github.com/vindagollc/osm-api/
Do have a look at the dev branch

@westnordost
Copy link
Member Author

westnordost commented Feb 27, 2024

Ah, cool, interesting! This is actually a fork/port of the Java library, so this clears the license question, it must be licensed under the identical license as the java library. Would you add the license information, too?

I can have a more thorough look through it later. Do you prefer I create issues for everything I find, or rather one issue in which I bundle the "review". Or some other method? It would be nice if there was a way where I could easily comment on and make suggestions on single lines. Maybe make a PR from master to a new empty branch that does not contain anything yet, so everything is a diff?

From a cursory look, it looks quite complete already! I haven't looked at the code in detail, and there seem to be still some open TODOs / WIP, but the overall structure looks good! That must have been quite a lot of work!

Some thoughts:

  • there are still a few example files around, e.g. Greeting.kt. Also, the OsmConnection class has some hardcoded basic authentication in there

  • the tests look incomplete. If you basically copied the Java library, couldn't you reuse (& convert to kotlin) the tests from the Java library instead of implementing them from scratch? If I remember correctly, the test coverage of the Java library has been very good

  • I see despite using kotlinx-serialization / io.github.pdvrieze.xmlutil you parse (and generate?) the XML response from the API by hand. With kotlinx-serialization it is not necessary (nor recommended, as far as I know) to do parsing by hand. You can simply annotate the data model how it should be (de)serialized. See https://kotlinlang.org/docs/serialization.html#serialize-and-deserialize-json for JSON, I would expect it to be somewhat similar for XML. Did you not know this, or is there some reason why you chose to parse it by hand?
    (Using this annotation-based approach not only reduced the XML parsing code, it should then of course reduce or eliminate the need for tests concerning the XML parsing)

  • Most data classes should probably rather be data classes. It is uncommon to have open classes.

  • I created the Java library almost ten years ago, in Java. There are a few things I would have done differently today, plus, since Kotlin is another language which has other structures and best practices and you use another library for communication with the OSM Api, it would make sense to do a few things differently architecture-wise. From the top of my head (i.e. incomplete) and without thoroughly thinking about it - so take it with a grain of salt:

    • OsmConnection used to hold/bundle the information necessary and the common logic necessary to make calls to the OSM API. I think with ktor http one would basically use HttpClient instead, because on the http client, you can already set the default user agent, default timeout etc. For common logic, static internal functions seem to be the better option.
    • The "Handler" classes is a callback-based API. In Kotlin, especially with coroutines, one would use Flows for that instead. Flows is an interesting topic, it's a bit of a read, but they are very interesting and helpful. The Ktor HttpClient being deep in the Kotlin world, I would expect that there is already something that facilitates this
    • That LatLon is an interface... I don't remember why I did that, probably because there used to be a Fixed1e7LatLon implementation to "save space" before I noticed that it was really slow. I think LatLon could just be a data class.
    • I am not sure if I would have created all those custom exception classes for different HTTP status codes if I reimplemented the library now. It has been a lot of work to maintain to keep up to date with current behavior of the OSM API as new status codes were introduced / documented. I expect it to continue to be an (unnecessary) effort
  • The port/fork looks very similar to the osmapi java library architecture-wise. I wonder if it would be an option to make the KMP version, once finished, the next major version of the java library to reduce maintenance effort overall(?) (no guarantees, not sure how much this library would differ from the original compatibility/migration-wise

@westnordost westnordost moved this from Todo to In Progress in iOS Port Feb 28, 2024
@westnordost westnordost moved this from In Progress to Done in iOS Port Mar 4, 2024
@westnordost westnordost moved this from Done to In Progress in iOS Port Mar 4, 2024
@westnordost westnordost removed the blocked blocked by another issue label Apr 7, 2024
@westnordost westnordost self-assigned this Jun 6, 2024
@westnordost westnordost moved this from In Progress to Blocked in iOS Port Jun 13, 2024
@github-project-automation github-project-automation bot moved this from Blocked to Done in iOS Port Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS necessary for iOS port
Projects
Status: Released
Development

Successfully merging a pull request may close this issue.

2 participants