rtdi / JDBCoData

An oData service for databases
Apache License 2.0
8 stars 5 forks source link

Feature/initial #11

Closed Bartman0 closed 1 year ago

Bartman0 commented 1 year ago

production version

wernerdaehn commented 1 year ago

I looked at the diffs, that is a lot. Hence need some help.

  1. The goal of this library is to be a library, not an application. A library uploaded to maven central and for others to consume. If I am not mistaken, you changed the goal to a docker container and removed the maven central upload (sonatype OSS)?
  2. I am coming from the SAP world and one database schema has 100'000 tables. Therefore I had one oData endpoint per table. Yes, this is not ideal as it prevents all advanced operations like oData expand but it keeps the $metadata document small. If I am not mistaken we can actually have both, just be either specifying /odata/schema or /odata/schema/table as endpoint. With your change it is always the first and never the latter, correct?

Suggestions? Clarifications?

Bartman0 commented 1 year ago

I closed the pull request, because I didn't intend to target your repo. I clicked too fast when creating the PR. I agree, this is a fundamental change and should not be incorporated in your repo.

You might want to have a look at the parsing code. I fixed a bug in BinaryExpression (should have IBooleanExpression too), and the charAt() in the group parsing should be a get() call.

protected void parse(CharBuffer in) throws ODataException {
    while (in.hasRemaining() && in.get(in.position()) != ')') {
        super.parse(in);
    }
    group = (IBooleanExpression) stack.pop();
}

Salesforce uses grouping in their requests a lot, and that resulted in indexing and parsing errors.

Bartman0 commented 1 year ago

I can only close this pull request. Is there a way to abandon it? I can't find it, but I am more familiar with gitlab and Azure DevOps.

Bartman0 commented 1 year ago

sorry, another comment. I also added test cases for that grouping parsing that you may want to cherry pick.

wernerdaehn commented 1 year ago

But you are okay if I try to merge your code? The idea would be that we do everything OData related here including environment variables and you can then build the container with just picking the library?

Bartman0 commented 1 year ago

yes, of course.

that approach would be wise to follow things going forward.

wernerdaehn commented 1 year ago

I have made the following changes:

  1. updated all libraries to the current version
  2. I kept the odata/tables/schema/tablename/ endpoint and it exposes a single table, the named table, and the entityset name is RS. Otherwise the url is schemaname/tablename/tablename which does not look nice.
  3. I created a new endpoint odata/schemas/schemaname/ which is one endpoint with all tables in it. This is using your naming for entity sets, namespaces etc
  4. I have added an embedded tomcat for testing. It is in its infancy but will allow us writing proper JUnit tests.

Next I will test if all works for me still.