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

Pass parameter "transaction" to the Parse Server #1090

Closed
wants to merge 2 commits into from
Closed

Pass parameter "transaction" to the Parse Server #1090

wants to merge 2 commits into from

Conversation

mfkenson
Copy link

#922

Did I miss anything?

@@ -230,6 +232,11 @@ const RESTController = {
}
}

const transaction = options.transaction;
if (transaction) {
payload.transaction = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to lint your code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite new to this and don't know what needs to be done exactly. Can you give me some advises how to get started?

Copy link
Contributor

@JeromeDeLeon JeromeDeLeon Feb 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just run npm run lint or yarn lint If you are using yarn in the command line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the Contributing Guide for all your testing and linting needs.

@davimacedo
Copy link
Member

Nice! Thanks for contributing. Could you please add a test case?

@dplewis
Copy link
Member

dplewis commented Mar 6, 2020

@davimacedo Can you take this over and add the test cases? You know more about how transactions in parse work better than anyone.

@davimacedo
Copy link
Member

Sure. I will work on this.

@dplewis
Copy link
Member

dplewis commented Mar 22, 2020

@davimacedo Any updates?

@codecov
Copy link

codecov bot commented Mar 22, 2020

Codecov Report

Merging #1090 into master will decrease coverage by 0.07%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1090      +/-   ##
=========================================
- Coverage   92.28%   92.2%   -0.08%     
=========================================
  Files          54      54              
  Lines        5235    5223      -12     
  Branches     1172    1172              
=========================================
- Hits         4831    4816      -15     
- Misses        404     407       +3
Impacted Files Coverage Δ
src/ParseObject.js 89.2% <25%> (-0.4%) ⬇️
src/RESTController.js 83.66% <66.66%> (-0.34%) ⬇️
src/ParseQuery.js 95.97% <0%> (-0.24%) ⬇️
src/CryptoController.js 100% <0%> (+21.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a33456...61594b4. Read the comment docs.

@davimacedo
Copy link
Member

Sorry for the delay. I am working on this now.

@davimacedo
Copy link
Member

@dplewis @mfkenson I started worked on the tests here but there is a problem with this implementation. In the case you have more objects to be saved/destroyed than the batch size, the SDK will split it in more than one batch and it will be generated two transactions in the server side. I think we can solve it using two different approaches:

  1. Change the sdk to never split into batches when the transaction option is set to true
  2. Throw an error message when the number of objects to be saved/destroyed is bigger than the batch size and the transaction option is set to true.

Thoughts?

@dplewis
Copy link
Member

dplewis commented Mar 24, 2020

@davimacedo I think that would be a great approach. I tried that here and ran into an issue. The issue is that we wouldn't be be able to find cycles in SaveAll if we never split into batches.

@davimacedo
Copy link
Member

I've just checked this problem and it will be very hard to be solved now. The not saved dependent objects need to go in a first batch because there is no warranty they will be executed in first place in the server side if we send them all together with the objects that depend on them. And if we have more batched sent to the server separately we cannot ensure the transaction. So, to simplify the solution and have a first version of this feature in the next release I suggest we simply do two verifications:

  • Verify if there is any not saved dependent object when transaction=true and throw an error in the case they exist.
  • Verify if there are more objects to be saved than the batch size and throw an error in the case there are.

Do you agree @dplewis ?

@dplewis
Copy link
Member

dplewis commented Mar 27, 2020

Thats the only solution I can come up with. We will have to document it well.

@andreasanta
Copy link

@mfkenson Can you please grant me access to this PR so I can fix linting and maybe try and perform some tests?

@davimacedo Any suggestions on how to start writing tests for this? :)

@davimacedo
Copy link
Member

Hi guys. I am working on this (to include the validations and the tests) but I found out a bug in Parse Server when deleting objects in a transaction. I am working now on a first PR to Parse Server before fixing this one here.

@andreasanta
Copy link

@davimacedo Thanks. Please let me know whether I can be of assistance.

@vahidalizad
Copy link
Contributor

vahidalizad commented Apr 15, 2021

Hi guys, Great work so far
I really had to use this feature so I came up with the following code:

function getServerUrlPath() {
  let serverUrl = SERVER_URL;
  if (serverUrl[serverUrl.length - 1] !== '/') {
    serverUrl += '/';
  }
  const url = serverUrl.replace(/https?:\/\//, '');
  return url.substr(url.indexOf('/'));
}

const MyCustom = Parse.Object.extend('MyCustom');
let obj1 = new MyCustom({name: 'just a name1'});
let obj2 = new MyCustom({name: 'just a name2'});

let batch = [obj1, obj2];
let options = {};

Parse._request(
  'POST',
  'batch',
  {
    requests: batch.map(obj => {
      const params = obj._getSaveParams();
      params.path = getServerUrlPath() + params.path;
      return params;
    }),
    transaction: true,
  },
  options
)
  .then(res => console.log('success', res))
  .catch(e => console.log('failed', e));

But there is something I don't understand when it fails, it sends up to REQUEST_ATTEMPT_LIMIT retries request but when I save only one file with await obj1.save() it only does it once and log it once. maybe this is because the batch request error does not return Parse.Error and the result has a code above 500.
Is this intended and I have to change REQUEST_ATTEMPT_LIMIT somehow?

@davimacedo
Copy link
Member

Maybe. You can set it via CoreManager.set('REQUEST_ATTEMPT_LIMIT', retry);

@vahidalizad
Copy link
Contributor

vahidalizad commented Apr 17, 2021

Parse.CoreManager.set('REQUEST_ATTEMPT_LIMIT', 1); changes every request retry times to 1 and it works, but there is no way to set a retry value for a single request.
but thanks anyway it works.
Maybe you could change the src/ParseServerRESTController.js in the definition of ParseServerRESTController like bellow to return the error like the normal batch requests

if (path === "/batch") {
    let initialPromise = Promise.resolve();
    if (data.transaction === true) {
    initialPromise = config.database.createTransactionalSession();
    }
    return initialPromise.then(() => {
    const promises = data.requests.map((request) => {
        return handleRequest(
        request.method,
        request.path,
        request.body,
        options,
        config
        ).then(
        (response) => {
            return Promise.resolve({ success: response });
        },
        (error) => {
            return Promise.resolve({
            error: { code: error.code, error: error.message },
            });
        }
        );
    });
    return Promise.all(promises)
        .catch((error) => {
        if (data.transaction === true) {
            return config.database.abortTransactionalSession().then(() => {
            throw error;
            });
        } else {
            throw error;
        }
        })
        .then((result) => {
        if (data.transaction === true) {
            let error = result.find((t) => t.error && t.code);
            if (error) {
            return config.database.abortTransactionalSession().then(() => {
                return error;
            });
            }
            return config.database.commitTransactionalSession().then(() => {
            return result;
            });
        } else {
            return result;
        }
        });
    });
}

In this way not only it would not retry it again, but it also shows the problem as well.
if you are agree with this and don't have the time for it I can create a pull request for it as well.

@davimacedo
Copy link
Member

That makes sense. Would you be willed to continue this PR?

@vahidalizad
Copy link
Contributor

vahidalizad commented Apr 19, 2021

That makes sense. Would you be willed to continue this PR?

Sorry my bad I just checked this FIX: Transaction should only abort when all promises finished #5878 PR and you fixed this problem there but it still lacks a retry option which is always 5 times but it does not log into the server which is ok I guess
I create a fork of the parse-server project and check this one again if it's ok on the master branch I'm gonna join you in this one if you haven't finished it yet.

@RahulLanjewar93
Copy link

@davimacedo Hi guys, any updates on this?

@mfkenson mfkenson deleted the feature/transaction branch February 10, 2023 02:37
@araskinwhichbnb
Copy link

Its Sep 2023 and there is an ominous comment above that says someone deleted this feature. Does that mean we are not going to get Transactional Integrity in the JS SDK?
@davimacedo

@RahulLanjewar93
Copy link

@mfkenson just wanted to know if there's any other pr/issue for reference since you closed this one.

@Llorx
Copy link

Llorx commented Jul 16, 2024

@mfkenson ???

@Llorx
Copy link

Llorx commented Jul 16, 2024

Hi guys. I am working on this (to include the validations and the tests) but I found out a bug in Parse Server when deleting objects in a transaction. I am working now on a first PR to Parse Server before fixing this one here.

@davimacedo Hey, have you fixed this problem? Can I create a new PR with the transaction: true flag now?

@vahidalizad
Copy link
Contributor

vahidalizad commented Aug 11, 2024

That makes sense. Would you be willed to continue this PR?

@davimacedo Thank you for your patience! I apologize for the delay. it took me three years to get back to this 😅. I was surprised that this hadn't been completed yet, so I went ahead and created a PR to address it.

Here’s the link to the PR: #2265.

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.

9 participants