Skip to content

datastore: Transaction sends non-transactional requests #204

Description

@ryanseys

Confusing that a Transaction object can send non-transaction requests. It would be better if this logic was abstracted into something like a "APIRequest" or something.

Instead of instantiating Dataset with a transaction object, it should just be a API request creator object.

Maybe I'm being too picky.

Activity

  1. stephenplusplus commented on Sep 12, 2014

    @stephenplusplus
    Contributor

    👍 it is a bit confusing at first. Maybe:

    function ApiRequest() {}
    ApiRequest.prototype.delete
    ApiRequest.prototype.get
    ApiRequest.prototype.mapQuery
    ApiRequest.prototype.makeReq
    ApiRequest.prototype.runQuery
    ApiRequest.prototype.save
    
    function Transaction() {}
    Transaction.prototype.begin
    Transaction.prototype.commit
    Transaction.prototype.finalize
    Transaction.prototype.rollback
    
    function Dataset() {}
    util.extend(Dataset, ApiRequest);

    We would just need to figure out how to get jsdoc to handle this.

  2. ryanseys commented on Sep 12, 2014

    @ryanseys
    ContributorAuthor

    Why not have Dataset just create an ApiRequest object and return that? We can use this to create requests for all the APIs in this library (much like is done in google-api-nodejs-client. Transaction can just be designed to make transactional ApiRequests.

  3. stephenplusplus commented on Sep 12, 2014

    @stephenplusplus
    Contributor

    Why not have Dataset just create an ApiRequest object and return that?

    👍 assuming you mean Dataset methods (.get, .whatever) return ApiRequest objects.

    Can you put together some rough code so I can better visualize how dataset/transaction would work with the new ApiRequest?

  4. ryanseys commented on Sep 12, 2014

    @ryanseys
    ContributorAuthor

    assuming you mean Dataset methods (.get, .whatever) return ApiRequest objects.

    Yes.

    Can you put together some rough code

    Yes. I'll try and get a rough implementation out this weekend.

  5. ryanseys commented on Sep 18, 2014

    @ryanseys
    ContributorAuthor

    Are we still doing this? I held back on implementing this because I wasn't sure what others thought. @silvolu @rakyll

  6. rakyll commented on Sep 18, 2014

    @rakyll
    Contributor

    I don't like the fact that non-transactional calls are abstracted as method's of a pseudo transaction either. @ryanseys, could you propose an API with ApiRequest objects? I don't fully understand how it will look from the user's perspective.

  7. ryanseys commented on Sep 19, 2014

    @ryanseys
    ContributorAuthor

    Uhh, the API should not change as far as the user is concerned. More just refactoring. Separation of responsibility.

  8. rakyll commented on Sep 19, 2014

    @rakyll
    Contributor

    SGTM.

  9. changed the title [-]Transaction sends non-transactional requests[/-] [+]datastore: Transaction sends non-transactional requests[/+] on Oct 5, 2014
  10. added
    api: datastoreIssues related to the Datastore API.
    type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.
    on Feb 2, 2015
  11. added this to the Datastore Stable milestone on Feb 2, 2015
  12. 52 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

🚨This issue needs some love.api: datastoreIssues related to the Datastore API.triage meI really want to be triaged.type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.

Type

No type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions