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

iD Support (and other stuff) #12

Merged
merged 52 commits into from
Jan 21, 2016
Merged

Conversation

mojodna
Copy link
Contributor

@mojodna mojodna commented Jan 4, 2016

This changes endpoints (and implements new ones) necessary to support the iD editor.

Of the changes, the map endpoint is probably the most significant, as it changes up the queries that fetch features (matching cgimap, the OSM C++ implementation of the map endpoint). Previously, nodes that were not part of ways were not being returned.

Fixes #11

/cc @olafveerman @dereklieu @danielfdsilva @kamicut

Allows osmosis to be used without allowIncorrectSchemaVersion=yes
(It's an LTS release, no other reason.)
Existing version was outputting correct info, but not at the URL that iD
expected.
Also handles changesets containing individual ways by turning them into
single-element arrays (change counts are similarly affected).
Necessary due to xml2json conversion.
This removes nodes from output to match OSM.
Updates pinned Node version to 5.3 (5+ required) to facilitate use of
Sets and the spread operator (for converting them to arrays).

The previous queryBbox implementation failed to load standalone nodes,
so I rewrote the DB queries to (largely) match what cgimap (the C++
implementation of the map endpoint) does (preferring to load
node/way/relation metadata in a batch).

This also simplifies rendering to XML by moving nd / member ordering
(defined by sequence_id) into SQL.
(Due to the use of the spread operator.)
Errors within Bluebird promises were proving difficult to debug and
since recent Node includes builtin Promises, it wasn't providing a whole
lot of value.
@dereklieu
Copy link
Contributor

Hey @mojodna, first off thanks for all the work here!

I've done some work off your branch. My goals were to preserve compatibility with original routes in addition to iD/OSM routes (api/0.6 route syntax), modify tests to pass with the new code and make fixes where applicable, and update the docker testing environment to run on node 5.3.

The code is on a new branch. Could you review and fast-forward into your branch if it still accomplishes your goals?

My other request is to please provide documentation and simple test coverage for the additional endpoints you've added, to cover possible future regressions. I'm seeing these:

/api/0.6/nodes
/api/0.6/ways
/api/capabilities
/api/user

We use apidocjs. Happy to help as much as I can with both documentation and test coverage.

@mojodna
Copy link
Contributor Author

mojodna commented Jan 12, 2016

Sweet. I think I'll have time to incorporate your changes later in the week and then we'll be on the same path once again.

@dereklieu
Copy link
Contributor

@mojodna great. I'll hold off on merging these changes until you do.

@mojodna
Copy link
Contributor Author

mojodna commented Jan 13, 2016

@dereklieu Fast-forwarded, etc, but I haven't written docs / tests just yet. That's next.

@mojodna
Copy link
Contributor Author

mojodna commented Jan 13, 2016

(And I'm seeing a bunch of test failures locally, so I want to sort those out before this gets merged.)

@dereklieu
Copy link
Contributor

@mojodna Cool. Let me know if you have any questions regarding any of the tests.

@mojodna
Copy link
Contributor Author

mojodna commented Jan 20, 2016

Existing tests pass now (locally). API docs are next.

@mojodna
Copy link
Contributor Author

mojodna commented Jan 21, 2016

@dereklieu this is ready to merge from my POV.

I haven't added new tests, but I'm willing to deal with breakage and introduce them when that happens. I'll also be adding some additional functionality (user info, history info in the db to be able to walk changesets) that will probably touch each of the endpoints I added, so I'll add tests at that point. (I'd like to get us synced up sooner rather than later.)

@dereklieu
Copy link
Contributor

Thanks @mojodna. The new feature list is minimal at this point, so I'm ok to sync up now and write tests for those later. Can't wait to see the new features/tests you write for that additional functionality.

I've spotted some minor fixes, which I'll open a PR for after I merge this. I'll also be doing some work to make sure /upload works again, so watch out for a PR.

dereklieu added a commit that referenced this pull request Jan 21, 2016
@dereklieu dereklieu merged commit 0bc8719 into developmentseed:develop Jan 21, 2016
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